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

Update virtualmachineimages service to SDK v2 #3843

Merged
merged 2 commits into from
Aug 19, 2023

Conversation

willie-yao
Copy link
Contributor

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Updates the virtualmachineimages service to use SDK v2.

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 #3569
Fixes #3473

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Update virtualmachineimages service to SDK v2

@willie-yao willie-yao force-pushed the sdkv2-vmimage-token branch 2 times, most recently from 898a27c to 14abea7 Compare August 15, 2023 18:14
@willie-yao willie-yao closed this Aug 15, 2023
@willie-yao willie-yao reopened this Aug 15, 2023
@willie-yao
Copy link
Contributor Author

/assign @mboersma

@k8s-ci-robot k8s-ci-robot added 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. labels Aug 15, 2023
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 15, 2023
@k8s-ci-robot
Copy link
Contributor

@willie-yao: Closed this PR.

In response to this:

/close

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
Copy link
Contributor

@willie-yao: Reopened this PR.

In response to this:

/reopen

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 reopened this Aug 15, 2023
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 15, 2023
@willie-yao willie-yao force-pushed the sdkv2-vmimage-token branch 2 times, most recently from 68a7422 to 9d19b85 Compare August 16, 2023 17:14
@Jont828
Copy link
Contributor

Jont828 commented Aug 16, 2023

@willie-yao Looks like you got a failing test btw

azure/scope/identity.go Outdated Show resolved Hide resolved
azure/scope/identity.go Outdated Show resolved Hide resolved
@willie-yao willie-yao force-pushed the sdkv2-vmimage-token branch from 9d19b85 to 1c9469b Compare August 16, 2023 17:55
@Jont828
Copy link
Contributor

Jont828 commented Aug 16, 2023

/assign

@willie-yao willie-yao force-pushed the sdkv2-vmimage-token branch 2 times, most recently from 3dcedd0 to 0fbd975 Compare August 16, 2023 21:04
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Patch coverage: 29.48% and project coverage change: -0.09% ⚠️

Comparison is base (2789eae) 55.49% compared to head (0a9e100) 55.41%.
Report is 26 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3843      +/-   ##
==========================================
- Coverage   55.49%   55.41%   -0.09%     
==========================================
  Files         188      189       +1     
  Lines       19456    19396      -60     
==========================================
- Hits        10797    10748      -49     
+ Misses       8057     8046      -11     
  Partials      602      602              
Files Changed Coverage Δ
azure/scope/clients.go 44.55% <0.00%> (-3.32%) ⬇️
azure/scope/cluster.go 56.81% <ø> (ø)
azure/scope/identity.go 35.71% <0.00%> (-1.50%) ⬇️
azure/scope/machinepool.go 29.43% <0.00%> (-2.01%) ⬇️
azure/scope/machine.go 50.40% <25.00%> (-0.42%) ⬇️
azure/services/virtualmachineimages/client.go 30.76% <36.36%> (-19.24%) ⬇️
azure/services/virtualmachineimages/images.go 81.95% <41.66%> (-2.43%) ⬇️
azure/services/virtualmachineimages/cache.go 75.40% <64.28%> (-7.93%) ⬇️

... and 9 files with indirect coverage changes

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

@mboersma
Copy link
Contributor

@willie-yao do you think it would be worth splitting this into two commits? One for the auth changes under azure/scope, another with everything else for the virtualmachineimages service. Just because it might make it easier to review now (and in the future, since several SDKv2 tasks might want to refer to this).

Just an idea, please ignore if it sounds like a hassle.

@willie-yao willie-yao force-pushed the sdkv2-vmimage-token branch from 0fbd975 to 9d745e8 Compare August 17, 2023 20:20
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 17, 2023
@willie-yao
Copy link
Contributor Author

do you think it would be worth splitting this into two commits?

@mboersma Done!!

@Jont828
Copy link
Contributor

Jont828 commented Aug 17, 2023

/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 17, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 10b5c338ce6231f1c03f23f7fedd8db2b9ccbfce

Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm
/assign @CecileRobertMichon

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

Can we update code comments in the modified interfaces to account for the added Token() func? It would be also helpful to make the distinction that Authorizer is used by sdk v1 services while Token is used by sdk v2 services in code comments as the redundancy might be confusing to a future reader

@willie-yao willie-yao force-pushed the sdkv2-vmimage-token branch from 48b29a5 to 0a9e100 Compare August 18, 2023 21:25
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2023
@CecileRobertMichon
Copy link
Contributor

/lgtm
/approve

Great work @willie-yao, I like the simple and elegant solution you ended up with!

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

LGTM label has been added.

Git tree hash: cb3ba262023a1334807a70c4f156b7a9394602b5

@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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2023
@Jont828
Copy link
Contributor

Jont828 commented Aug 19, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit de9f501 into kubernetes-sigs:main Aug 19, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.11 milestone Aug 19, 2023
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 Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
Archived in project
5 participants