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

Make managedcluster reconcile and delete async #2168

Merged
merged 1 commit into from
May 24, 2022
Merged

Make managedcluster reconcile and delete async #2168

merged 1 commit into from
May 24, 2022

Conversation

CecileRobertMichon
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon commented Mar 14, 2022 •
edited
Loading

What type of PR is this?
/kind cleanup

What this PR does / why we need it: Implements #1702 for managedclusters. Also separates managedcontrolplane and managedmachinepool scopes, adds tests, and moves validation to webhooks as much as possible.

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

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:

Make managedcluster reconcile and delete async

@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. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 14, 2022
@k8s-ci-robot k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Mar 14, 2022
@CecileRobertMichon CecileRobertMichon mentioned this pull request Mar 15, 2022
Closed
@CecileRobertMichon CecileRobertMichon force-pushed the async-managedclusters branch 2 times, most recently from b55a41e to aa85d7d Compare March 17, 2022 00:19
@CecileRobertMichon CecileRobertMichon force-pushed the async-managedclusters branch 3 times, most recently from 359d2c7 to 446458d Compare March 29, 2022 00:45
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2022
@CecileRobertMichon CecileRobertMichon force-pushed the async-managedclusters branch 2 times, most recently from 9da7038 to ca96944 Compare March 29, 2022 17:03
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2022
@CecileRobertMichon CecileRobertMichon force-pushed the async-managedclusters branch 5 times, most recently from adfd9ae to a476682 Compare April 5, 2022 23:43
@CecileRobertMichon CecileRobertMichon changed the title [WIP] Make managedcluster reconcile and delete async Make managedcluster reconcile and delete async Apr 5, 2022
@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 Apr 5, 2022
@CecileRobertMichon CecileRobertMichon force-pushed the async-managedclusters branch 2 times, most recently from 1d2d2ea to 92d9c9b Compare April 5, 2022 23:58
@CecileRobertMichon
Copy link
Contributor Author

CecileRobertMichon commented Apr 5, 2022 •
edited
Loading

/cc @shysank @Jont828 @zmalik @alexeldeib @jackfrancis this should be ready for review...

I realize it's a very large PR so please call me out on any scope creeps that I should try and decompose. I ended up having to a bit of refactoring of cluster and agentpool specs and moving some validation errors to webhooks so getting the spec itself wouldn't return errors. Kept the commits unsquashed for now so it should hopefully help with history and thought process.

@CecileRobertMichon CecileRobertMichon force-pushed the async-managedclusters branch 2 times, most recently from 5efaa83 to e2c6d18 Compare May 12, 2022 23:40
@CecileRobertMichon
Copy link
Contributor Author

@Jont828 thanks for the review! I've addressed most of your comments, PTAL. I kept the new changes as a separate commit, let me know if this looks good and I can squash.

@CecileRobertMichon
Copy link
Contributor Author

/retest

@CecileRobertMichon
Copy link
Contributor Author

@Jont828 @shysank did you have any other comments? Let me know when I can squash

1 similar comment
@CecileRobertMichon
Copy link
Contributor Author

@Jont828 @shysank did you have any other comments? Let me know when I can squash

Jont828
Jont828 reviewed May 18, 2022
View reviewed changes
Copy link
Contributor

@Jont828 Jont828 left a comment

Choose a reason for hiding this comment

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

Small thing but other than that I think we're good to go!

exp/api/v1beta1/azuremanagedcontrolplane_webhook.go Outdated Show resolved Hide resolved
@netlify
Copy link

netlify bot commented May 19, 2022 •
edited
Loading

👷 Deploy request for kubernetes-sigs-cluster-api-provider-azure pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2d95950

@CecileRobertMichon
Copy link
Contributor Author

Squashed

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 20, 2022 •
edited
Loading

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-azure-apidiff 2d95950 link false /test pull-cluster-api-provider-azure-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@CecileRobertMichon
Copy link
Contributor Author

@Jont828 @mboersma @shysank PTAL, addressed all comments and squashed

@Jont828
Copy link
Contributor

Jont828 commented May 20, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2022
@CecileRobertMichon
Copy link
Contributor Author

@mboersma @shysank any objections on merging this one?

@mboersma
Copy link
Contributor

lgtm! Please merge, your patience is admirable.

@mboersma
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma

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 May 24, 2022
@k8s-ci-robot k8s-ci-robot merged commit f277598 into kubernetes-sigs:main May 24, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone May 24, 2022
@jackfrancis
Copy link
Contributor

image

@CecileRobertMichon CecileRobertMichon mentioned this pull request Jul 6, 2022
Merged
3 tasks
This was referenced Sep 26, 2022
Closed
Closed
@CecileRobertMichon CecileRobertMichon deleted the async-managedclusters branch February 17, 2023 23:27
@jackfrancis jackfrancis mentioned this pull request Jan 31, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonasingh46 sonasingh46 sonasingh46 left review comments

@Jont828 Jont828 Jont828 left review comments

@shysank shysank shysank left review comments

@alexeldeib alexeldeib alexeldeib left review comments

@jackfrancis jackfrancis jackfrancis requested changes

@mboersma mboersma mboersma approved these changes

@nader-ziada nader-ziada Awaiting requested review from nader-ziada

Assignees

@Jont828 Jont828

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.

async managedclusters
8 participants
@CecileRobertMichon @alexeldeib @jackfrancis @k8s-ci-robot @Jont828 @mboersma @shysank @sonasingh46