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.

Sign up for GitHub

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

Jump to bottom

Implement the reconciliation loop for GCPManagedControlPlane (GKE Part 3) #770

Merged
merged 7 commits into from
Jan 25, 2023
Merged

Implement the reconciliation loop for GCPManagedControlPlane (GKE Part 3) #770

merged 7 commits into from
Jan 25, 2023

Conversation

richardchen331
Copy link
Contributor

@richardchen331 richardchen331 commented Nov 30, 2022 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
This change adds the reconciliation logic for the GCPManagedControlPlane.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Relates #512

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Add the reconciliation logic for **GCPManagedControlPlane**

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 30, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @richardchen331. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 30, 2022
@richardchen331 richardchen331 changed the title Implement the reconciliation loop for GCPManagedControlPlane [WIP] Implement the reconciliation loop for GCPManagedControlPlane Nov 30, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2022
@richardcase
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 14, 2022
@richardcase richardcase changed the title [WIP] Implement the reconciliation loop for GCPManagedControlPlane [WIP] Implement the reconciliation loop for GCPManagedControlPlane (GKE Part 3) Jan 11, 2023
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 12, 2023
@richardchen331 richardchen331 changed the title [WIP] Implement the reconciliation loop for GCPManagedControlPlane (GKE Part 3) Implement the reconciliation loop for GCPManagedControlPlane (GKE Part 3) Jan 12, 2023
@richardchen331 richardchen331 marked this pull request as ready for review January 12, 2023 18:26
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 12, 2023
@richardchen331
Copy link
Contributor Author

@cpanato @richardcase I rebased this PR and it's also ready now, please take a look and let me know if you have any feedback, thanks!

@richardchen331
Copy link
Contributor Author

As the GKE support isn't released we can ignore apidiff errors:

/override pull-cluster-api-provider-gcp-apidiff

@k8s-ci-robot
Copy link
Contributor

@richardchen331: richardchen331 unauthorized: /override is restricted to Repo administrators.

In response to this:

As the GKE support isn't released we can ignore apidiff errors:

/override pull-cluster-api-provider-gcp-apidiff

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@richardchen331
Copy link
Contributor Author

@richardchen331: richardchen331 unauthorized: /override is restricted to Repo administrators.

@richardcase could you help override this? i don't seems to have permission. thanks!

@richardcase
Copy link
Member

/override pull-cluster-api-provider-gcp-apidiff

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, richardchen331

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2023
@richardcase
Copy link
Member

/override pull-cluster-api-provider-gcp-apidiff

@k8s-ci-robot
Copy link
Contributor

@richardcase: Overrode contexts on behalf of richardcase: pull-cluster-api-provider-gcp-apidiff

In response to this:

/override pull-cluster-api-provider-gcp-apidiff

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

richardcase
richardcase reviewed Jan 17, 2023
View reviewed changes
Copy link
Member

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

Looking really good @richardchen331, great work.

A few minor comments, feel free to ping me if i can help.

cloud/scope/managedcontrolplane.go Outdated Show resolved Hide resolved
cloud/scope/managedcontrolplane.go Outdated Show resolved Hide resolved
cloud/scope/managedcontrolplane.go Outdated Show resolved Hide resolved
cloud/services/container/clusters/reconcile.go Outdated Show resolved Hide resolved
cloud/services/container/clusters/reconcile.go Outdated Show resolved Hide resolved
exp/api/v1beta1/gcpmanagedcontrolplane_types.go Show resolved Hide resolved
cloud/services/container/clusters/reconcile.go Outdated Show resolved Hide resolved
exp/controllers/gcpmanagedcontrolplane_controller.go Outdated Show resolved Hide resolved
exp/controllers/gcpmanagedcontrolplane_controller.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@richardchen331
Copy link
Contributor Author

@richardcase Thanks for the review. I updated the PR according to your comments. Please take a look and let me know if you have any questions. Thanks!

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 19, 2023
@richardcase
Copy link
Member

As the GKE implementation isn't released:

/override pull-cluster-api-provider-gcp-apidiff

@k8s-ci-robot
Copy link
Contributor

@richardcase: Overrode contexts on behalf of richardcase: pull-cluster-api-provider-gcp-apidiff

In response to this:

As the GKE implementation isn't released:

/override pull-cluster-api-provider-gcp-apidiff

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

richardcase
richardcase reviewed Jan 21, 2023
View reviewed changes
Copy link
Member

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

This is looking really good.

Only a few comments, after they are resolved we can merge.

cloud/scope/credentials.go Outdated Show resolved Hide resolved
cloud/services/container/clusters/reconcile.go Outdated Show resolved Hide resolved
cloud/services/container/clusters/reconcile.go Outdated Show resolved Hide resolved
cloud/services/container/clusters/reconcile.go Show resolved Hide resolved
cloud/scope/managedcontrolplane.go Outdated Show resolved Hide resolved
exp/api/v1beta1/gcpmanagedcontrolplane_types.go Show resolved Hide resolved
exp/controllers/gcpmanagedcluster_controller.go Outdated Show resolved Hide resolved
exp/controllers/gcpmanagedcontrolplane_controller.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@richardchen331
Copy link
Contributor Author

@richardcase Thanks for all the feedback! I updated the PR accordingly. Please take a look and let me know if you have other comments. Thanks!

@richardcase
Copy link
Member

/override pull-cluster-api-provider-gcp-apidiff

@k8s-ci-robot
Copy link
Contributor

@richardcase: Overrode contexts on behalf of richardcase: pull-cluster-api-provider-gcp-apidiff

In response to this:

/override pull-cluster-api-provider-gcp-apidiff

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@richardcase
Copy link
Member

Ideally we'd squash commits to tell a story but we've done a few rounds of reviews so:

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2023
@k8s-ci-robot k8s-ci-robot merged commit 517caab into kubernetes-sigs:main Jan 25, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.1.0 milestone Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpanato cpanato cpanato requested changes

@richardcase richardcase Awaiting requested review from richardcase

Assignees

@richardcase richardcase

@cpanato cpanato

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@richardchen331 @k8s-ci-robot @richardcase @cpanato