Skip to content

Convert natgateways service to SDKv2#3858

Merged
k8s-ci-robot merged 2 commits into
kubernetes-sigs:mainfrom
mboersma:sdk-v2-nat-gateways
Aug 24, 2023
Merged

Convert natgateways service to SDKv2#3858
k8s-ci-robot merged 2 commits into
kubernetes-sigs:mainfrom
mboersma:sdk-v2-nat-gateways

Conversation

@mboersma
Copy link
Copy Markdown
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 natgateways service to SDKv2 and a new asyncpoller framework for long-running operations.

Which issue(s) this PR fixes:

Fixes #3442
Fixes #3441

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
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 17, 2023 •
edited
Loading

Codecov Report

Patch coverage: 47.59% and project coverage change: +0.14% 🎉

Comparison is base (6232f31) 55.41% compared to head (517cfe2) 55.56%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3858      +/-   ##
==========================================
+ Coverage   55.41%   55.56%   +0.14%     
==========================================
  Files         189      190       +1     
  Lines       19420    19523     +103     
==========================================
+ Hits        10762    10848      +86     
- Misses       8055     8066      +11     
- Partials      603      609       +6
Files Changed Coverage Δ
azure/services/natgateways/client.go 0.00% <0.00%> (ø)
azure/services/natgateways/spec.go 4.34% <0.00%> (+0.26%) ⬆️
azure/services/natgateways/natgateways.go 76.00% <18.18%> (-4.29%) ⬇️
controllers/azurecluster_reconciler.go 69.64% <40.00%> (-1.66%) ⬇️
azure/services/asyncpoller/asyncpoller.go 74.56% <74.56%> (ø)

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

@mboersma
Copy link
Copy Markdown
Contributor Author

mboersma commented Aug 17, 2023

/hold

This needs to be rebased on the auth changes in #3843.

@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 17, 2023
@mboersma mboersma force-pushed the sdk-v2-nat-gateways branch from 150a8bd to cb15615 Compare August 18, 2023 21:18
@mboersma
Copy link
Copy Markdown
Contributor Author

mboersma commented Aug 18, 2023

I rebased this (temporarily) on #3843 and made the necessary changes to use that apporach for authentication.

@mboersma mboersma force-pushed the sdk-v2-nat-gateways branch from cb15615 to e3fdaa5 Compare August 19, 2023 15:20
@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 22, 2023
@mboersma mboersma force-pushed the sdk-v2-nat-gateways branch from e3fdaa5 to 94e6961 Compare August 22, 2023 13:48
@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 22, 2023
@mboersma
Copy link
Copy Markdown
Contributor Author

mboersma commented Aug 22, 2023 •
edited
Loading

This is ready for review if anyone has time.

But I'd like to get #3857 fully working as well before we merge this, so we have high confidence in the asyncpoller framework.

nojnhuh
nojnhuh reviewed Aug 22, 2023
View reviewed changes
Comment thread azure/services/natgateways/client.go Outdated
Comment thread azure/services/asyncpoller/asyncpoller.go Outdated
Comment thread azure/services/asyncpoller/interfaces.go Outdated
Comment thread azure/services/natgateways/client.go Outdated
Comment thread azure/services/asyncpoller/asyncpoller_test.go Outdated
Comment thread azure/services/asyncpoller/asyncpoller_test.go Outdated
Comment thread azure/services/natgateways/client.go Outdated
@mboersma
Copy link
Copy Markdown
Contributor Author

mboersma commented Aug 22, 2023

/hold

For more reviews, testing, and eventual squash.

nojnhuh
nojnhuh reviewed Aug 22, 2023
View reviewed changes
Comment thread azure/services/asyncpoller/asyncpoller.go Outdated
Comment thread azure/services/asyncpoller/asyncpoller.go Outdated
@mboersma mboersma force-pushed the sdk-v2-nat-gateways branch from 94e8d36 to 8d2b393 Compare August 23, 2023 01:41
nojnhuh
nojnhuh reviewed Aug 23, 2023
View reviewed changes
Comment thread azure/services/natgateways/spec.go Outdated
nojnhuh
nojnhuh reviewed Aug 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2023
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Aug 23, 2023

LGTM label has been added.

DetailsGit tree hash: 76062a9f212e317205384db33358264cca5a43ed

@mboersma mboersma force-pushed the sdk-v2-nat-gateways branch from d049a49 to 517cfe2 Compare August 23, 2023 20:28
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2023
@k8s-ci-robot k8s-ci-robot requested a review from nojnhuh August 23, 2023 20:28
@mboersma
Copy link
Copy Markdown
Contributor Author

mboersma commented Aug 23, 2023

I squashed the commits and added some additional unit test paths through CreateOrUpdateAsync.

The hope is that the second commit here 517cfe2 will serve as a reference for others to convert services to SDKv2 / asyncpoller. Please keep that in mind and suggest improvements, additional comments, etc.

@nojnhuh
Copy link
Copy Markdown
Contributor

nojnhuh commented Aug 23, 2023

LGTM pending review from at least one other person.

willie-yao
willie-yao approved these changes Aug 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for your hard work on this @mboersma
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2023
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Aug 23, 2023

LGTM label has been added.

DetailsGit tree hash: f5d9d7788fdbac3e7499b4080074538b675873d2

@mboersma
Copy link
Copy Markdown
Contributor Author

mboersma commented Aug 23, 2023

/retest

Looks like a provisioning flake in ci-entrypoint: timeout # Wait for the kubeconfig to become available.

@nojnhuh
Copy link
Copy Markdown
Contributor

nojnhuh commented Aug 24, 2023

/lgtm
/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Aug 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nojnhuh

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

The pull request process is described here

Details 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 Aug 24, 2023
@mboersma
Copy link
Copy Markdown
Contributor Author

mboersma commented Aug 24, 2023

/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 24, 2023
@k8s-ci-robot k8s-ci-robot merged commit e812eae into kubernetes-sigs:main Aug 24, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.11 milestone Aug 24, 2023
@mboersma mboersma deleted the sdk-v2-nat-gateways branch August 24, 2023 00:49
@willie-yao willie-yao mentioned this pull request Aug 24, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@willie-yao willie-yao willie-yao approved these changes

@Jont828 Jont828 Awaiting requested review from Jont828

@jsturtevant jsturtevant Awaiting requested review from jsturtevant

@nojnhuh nojnhuh Awaiting requested review from nojnhuh

Assignees

@nojnhuh nojnhuh

@willie-yao willie-yao

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

CAPZ Planning
Archived in project

Milestone

v1.11

Development

Successfully merging this pull request may close these issues.

Update NAT gateways service to SDK v2 Adapt async framework to SDK v2

4 participants

@mboersma @k8s-ci-robot @nojnhuh @willie-yao