Skip to content

Convert inboundnatrules service to SDKv2#3882

Merged
k8s-ci-robot merged 1 commit into
kubernetes-sigs:mainfrom
mboersma:sdk-v2-inboundnatrules
Aug 31, 2023
Merged

Convert inboundnatrules service to SDKv2#3882
k8s-ci-robot merged 1 commit into
kubernetes-sigs:mainfrom
mboersma:sdk-v2-inboundnatrules

Conversation

@mboersma
Copy link
Copy Markdown
Contributor

@mboersma mboersma commented Aug 24, 2023 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Converts the inboundnatrules service to azure-sdk-for-go version 2 and the asyncpoller framework for long-running operations.

Which issue(s) this PR fixes:

Fixes #3881

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 24, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 24, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 24, 2023 •
edited
Loading

Codecov Report

Patch coverage: 10.44% and project coverage change: +0.16% 🎉

Comparison is base (134117f) 55.78% compared to head (3ef6caa) 55.95%.
Report is 2 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3882      +/-   ##
==========================================
+ Coverage   55.78%   55.95%   +0.16%     
==========================================
  Files         190      190              
  Lines       19493    19477      -16     
==========================================
+ Hits        10874    10898      +24     
+ Misses       8005     7965      -40     
  Partials      614      614
Files Changed Coverage Δ
azure/services/inboundnatrules/client.go 0.00% <0.00%> (ø)
controllers/azuremachine_reconciler.go 43.24% <0.00%> (-2.48%) ⬇️
azure/services/inboundnatrules/inboundnatrules.go 74.07% <9.09%> (-3.85%) ⬇️
azure/services/inboundnatrules/spec.go 90.69% <100.00%> (+60.46%) ⬆️

... and 1 file with indirect coverage changes

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

@mboersma mboersma added this to the v1.11 milestone Aug 24, 2023
@mboersma
Copy link
Copy Markdown
Contributor Author

mboersma commented Aug 24, 2023

/test pull-cluster-api-provider-azure-e2e-workload-upgrade
/test pull-cluster-api-provider-azure-e2e-apiversion-upgrade
/test pull-cluster-api-provider-azure-e2e-optional

@mboersma
Copy link
Copy Markdown
Contributor Author

mboersma commented Aug 24, 2023

/test pull-cluster-api-provider-azure-apiversion-upgrade

willie-yao
willie-yao reviewed Aug 24, 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.

Overall this looks great! Just a minor comment from my end

Comment thread azure/services/natgateways/client.go
willie-yao
willie-yao reviewed Aug 24, 2023
View reviewed changes
Comment thread azure/services/inboundnatrules/spec.go
willie-yao
willie-yao reviewed Aug 24, 2023
View reviewed changes
Comment thread azure/services/inboundnatrules/client.go Outdated
Comment thread azure/services/inboundnatrules/client.go Outdated
@mboersma mboersma force-pushed the sdk-v2-inboundnatrules branch 2 times, most recently from 59afd83 to cdfb42e Compare August 24, 2023 23:01
@mboersma mboersma mentioned this pull request Aug 24, 2023
Closed
@mboersma mboersma force-pushed the sdk-v2-inboundnatrules branch from cdfb42e to 154edc2 Compare August 24, 2023 23:34
@mboersma
Copy link
Copy Markdown
Contributor Author

mboersma commented Aug 25, 2023

The only test this has failed is the workload-upgrade job, on known bug #3875.

nojnhuh
nojnhuh reviewed Aug 30, 2023
View reviewed changes
Comment thread azure/services/inboundnatrules/client.go Outdated
Comment on lines +33 to +36
List(context.Context, string, string) (result []armnetwork.InboundNatRule, err error)
Get(context.Context, azure.ResourceSpecGetter) (result interface{}, err error)
CreateOrUpdateAsync(context.Context, azure.ResourceSpecGetter, interface{}) (result interface{}, future azureautorest.FutureAPI, err error)
DeleteAsync(context.Context, azure.ResourceSpecGetter) (future azureautorest.FutureAPI, err error)
IsDone(context.Context, azureautorest.FutureAPI) (isDone bool, err error)
Result(context.Context, azureautorest.FutureAPI, string) (result interface{}, err error)
CreateOrUpdateAsync(context.Context, azure.ResourceSpecGetter, string, interface{}) (result interface{}, poller *runtime.Poller[armnetwork.InboundNatRulesClientCreateOrUpdateResponse], err error)
DeleteAsync(context.Context, azure.ResourceSpecGetter, string) (poller *runtime.Poller[armnetwork.InboundNatRulesClientDeleteResponse], err error)
Copy link
Copy Markdown
Contributor

@nojnhuh nojnhuh Aug 30, 2023

Choose a reason for hiding this comment

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

Not strictly related to these changes, but it looks like List is the only method called on the interface type, so I think the others can be removed.

@willie-yao
Copy link
Copy Markdown
Contributor

willie-yao commented Aug 30, 2023

/lgtm
/retest

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

k8s-ci-robot commented Aug 30, 2023

LGTM label has been added.

DetailsGit tree hash: 21fa1c9b022d72057f9aaee191861a495c666435

@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 30, 2023
@mboersma mboersma force-pushed the sdk-v2-inboundnatrules branch from 154edc2 to 84a4ccd Compare August 31, 2023 01:41
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 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 31, 2023
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Aug 31, 2023 •
edited
Loading

@mboersma: 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-e2e-workload-upgrade f1966c9 link false /test pull-cluster-api-provider-azure-e2e-workload-upgrade

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.

Details

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.

@mboersma mboersma force-pushed the sdk-v2-inboundnatrules branch from 84a4ccd to ab89b14 Compare August 31, 2023 02:13
@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 31, 2023
@mboersma mboersma force-pushed the sdk-v2-inboundnatrules branch from ab89b14 to 3ef6caa Compare August 31, 2023 11:49
@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 31, 2023
nojnhuh
nojnhuh reviewed Aug 31, 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
/approve

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

k8s-ci-robot commented Aug 31, 2023

LGTM label has been added.

DetailsGit tree hash: 655a15ce6c98794bb3477474b5ac1eedac4d3618

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Aug 31, 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 31, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9660a20 into kubernetes-sigs:main Aug 31, 2023
@mboersma mboersma deleted the sdk-v2-inboundnatrules branch August 31, 2023 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nojnhuh nojnhuh nojnhuh left review comments

@marosset marosset Awaiting requested review from marosset

@willie-yao willie-yao Awaiting requested review from willie-yao

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

CAPZ Planning
Archived in project

Milestone

v1.11

Development

Successfully merging this pull request may close these issues.

Update Inbound NAT rules service to SDK v2

4 participants

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