Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gcp : add alloydb group #373

Merged
merged 5 commits into from
Sep 7, 2023
Merged

Conversation

jcockbain
Copy link
Contributor

@jcockbain jcockbain commented Aug 25, 2023

Description of your changes

Fixes #371

Adds resources for:

google_alloydb_backup
google_alloydb_instance
google_alloydb_cluster

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

I deployed instances of the 3 new kinds to to a minikube cluster. These were based on the generated examples, adapted for my test GCP project.

I then used make run, which created the expected resources in my project.

Each of these resource became "ready". Deleting the k8s resources deletes the resources from GCP.

Cluster: https://github.com/upbound/provider-gcp/actions/runs/6110391551
Screenshot from 2023-09-06 17-48-49

Instance:
Screenshot from 2023-09-07 11-44-35

Backup:
Screenshot from 2023-09-07 13-46-57

@Upbound-CLA
Copy link

Upbound-CLA commented Aug 25, 2023

CLA assistant check
All committers have signed the CLA.

@turkenf
Copy link
Collaborator

turkenf commented Aug 31, 2023

/test-examples="examples/alloydb/instance.yaml"

@jcockbain
Copy link
Contributor Author

jcockbain commented Sep 4, 2023

@turkenf are you able to assist with the failure on ci/check-diff? Running make check-diff from my workstation passes fine.

@turkenf
Copy link
Collaborator

turkenf commented Sep 4, 2023

Hi @jcockbain,

After running make generate there seem to be changes you didn't commit, can you run make generate again and commit all the changes?

Screenshot 2023-09-04 at 16 11 09

@jcockbain jcockbain force-pushed the add-alloydb branch 3 times, most recently from ace6a9e to 615dcc3 Compare September 5, 2023 10:11
@jcockbain
Copy link
Contributor Author

@turkenf thanks. I have updated my branch, and changed the commits to show the manual and then the generated changes. make generate does not create any new files on my branch now, so hopefully the CI will be okay.

@jcockbain
Copy link
Contributor Author

Hi @turkenf, I run make generate on my machine and it passes fine - but the CI job is still failing. Any advice on what I could be missing here?

@turkenf
Copy link
Collaborator

turkenf commented Sep 5, 2023

@jcockbain, could you please rebase your branch to the main, run make generate, and commit all the changes again?

@jcockbain
Copy link
Contributor Author

Thanks @turkenf, there were extra files generated after rebasing.

Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcockbain, many thanks for your contribution. I left a few comments for you to consider.

Could you please fill in how it was tested after the last changes for each resource as here?

config/externalname.go Outdated Show resolved Hide resolved
config/externalname.go Outdated Show resolved Hide resolved
examples/alloydb/cluster.yaml Outdated Show resolved Hide resolved
examples/alloydb/backup.yaml Outdated Show resolved Hide resolved
examples/alloydb/instance.yaml Outdated Show resolved Hide resolved
@jcockbain
Copy link
Contributor Author

Thanks very much for your comments @turkenf, I have attempted to resolve them all in the above commit.

I also retested with the examples and added screenshots to the summary of this PR.

@jcockbain
Copy link
Contributor Author

One thing I am not certain on is what to name example resources, for my testing I gave the resources different names as default is already used for some of the resources in my test project. So I left these test names in the example manifests, but I can change them back to the generated ones if that would be better.

Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the quick update @jcockbain, the names look good for now, they can stay that way. I just left two minor comments that fix the example-ids.

examples/alloydb/backup.yaml Outdated Show resolved Hide resolved
examples/alloydb/instance.yaml Outdated Show resolved Hide resolved
@turkenf
Copy link
Collaborator

turkenf commented Sep 7, 2023

/test-examples="examples/alloydb/cluster.yaml"

1 similar comment
@turkenf
Copy link
Collaborator

turkenf commented Sep 7, 2023

/test-examples="examples/alloydb/cluster.yaml"

Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcockbain, Thank you again for your contribution and clean work. LGTM.

@turkenf turkenf merged commit 72c0894 into crossplane-contrib:main Sep 7, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request for alloydb resources
3 participants