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

Convert managedclusters service to SDKv2 #3857

Merged
merged 4 commits into from
Aug 30, 2023

Conversation

mboersma
Copy link
Contributor

@mboersma mboersma commented Aug 17, 2023

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Converts the managedclusters service and the agentpools service to use SDKv2 and the asyncpoller framework for long-running operations.

Which issue(s) this PR fixes:

Fixes #3861
Refs #3409

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 17, 2023
@k8s-ci-robot k8s-ci-robot added 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 Aug 17, 2023
@mboersma
Copy link
Contributor Author

/retitle [WIP] Convert managedclusters service to SDKv2

There are still a couple // TODOs here, it needs to be rebased on #3843, and there is an issue where the AzureManagedCluster stays in Provisioning state although the infrastructure is actually ready.

@k8s-ci-robot k8s-ci-robot changed the title Convert managedclusters service to SDKv2 [WIP] Convert managedclusters service to SDKv2 Aug 17, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 17, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2023
@mboersma mboersma force-pushed the sdk-v2-aks branch 2 times, most recently from dff3b0e to 6b286b9 Compare August 21, 2023 19:33
@mboersma mboersma force-pushed the sdk-v2-aks branch 4 times, most recently from da6b84d to 02c1f70 Compare August 23, 2023 23:46
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2023
@mboersma mboersma force-pushed the sdk-v2-aks branch 2 times, most recently from 629dcb2 to 7e45647 Compare August 24, 2023 19:45
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Patch coverage: 40.21% and project coverage change: +0.18% 🎉

Comparison is base (b709abf) 55.59% compared to head (3965331) 55.77%.

❗ Current head 3965331 differs from pull request most recent head 9fd45ec. Consider uploading reports for the commit 9fd45ec to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3857      +/-   ##
==========================================
+ Coverage   55.59%   55.77%   +0.18%     
==========================================
  Files         190      190              
  Lines       19507    19475      -32     
==========================================
+ Hits        10844    10863      +19     
+ Misses       8056     8004      -52     
- Partials      607      608       +1     
Files Changed Coverage Δ
azure/services/agentpools/client.go 0.00% <0.00%> (ø)
azure/services/managedclusters/client.go 0.00% <0.00%> (ø)
controllers/azuremanagedcontrolplane_controller.go 40.66% <0.00%> (ø)
controllers/azuremanagedmachinepool_controller.go 55.00% <0.00%> (ø)
controllers/azuremanagedmachinepool_reconciler.go 53.06% <0.00%> (-2.26%) ⬇️
azure/services/agentpools/agentpools.go 69.38% <23.07%> (-6.17%) ⬇️
azure/services/managedclusters/managedclusters.go 73.13% <23.07%> (-4.65%) ⬇️
azure/services/managedclusters/spec.go 50.49% <43.79%> (-0.13%) ⬇️
controllers/azuremanagedcontrolplane_reconciler.go 38.02% <50.00%> (-0.21%) ⬇️
azure/services/agentpools/spec.go 59.31% <71.42%> (-1.17%) ⬇️
... and 3 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold for squash

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2023
@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 29, 2023
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/hold cancel
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 29, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 054bbe9c9f424f530ac6075b88758a75b60a5882

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 29, 2023
@mboersma
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2023
@CecileRobertMichon
Copy link
Contributor

/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts

flake

@mboersma
Copy link
Contributor Author

/hold

The AKS is test is taking an unusually long time, so even if it passes I'd like to verify that it's an outlier.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2023
@mboersma
Copy link
Contributor Author

/retest

@mboersma
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-aks

Let's see another green run.

@CecileRobertMichon
Copy link
Contributor

CecileRobertMichon commented Aug 30, 2023

last aks run failed

/retest

@CecileRobertMichon
Copy link
Contributor

/test pull-cluster-api-provider-azure-e2e-aks

@CecileRobertMichon
Copy link
Contributor

@mboersma I checked the two previous e2e failures, they happened at two different places in the test so I don't think there's a pattern

@mboersma
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2023
@k8s-ci-robot k8s-ci-robot merged commit 02ca989 into kubernetes-sigs:main Aug 30, 2023
@mboersma mboersma deleted the sdk-v2-aks branch August 30, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Update managedclusters service to SDK v2
5 participants