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

Refactor: migrate disk client to track2 sdk #2155

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Refactor: migrate disk client to track2 sdk #2155

merged 1 commit into from
Jan 19, 2024

Conversation

MartinForReal
Copy link
Contributor

@MartinForReal MartinForReal commented Jan 15, 2024 •
edited
Loading

What type of PR is this?
Refactor: migrate disk client to track2 sdk

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Requirements:

Special notes for your reviewer:

Release note:

none

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jan 15, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2024
@MartinForReal MartinForReal marked this pull request as ready for review January 15, 2024 12:35
@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 Jan 15, 2024
@MartinForReal
Copy link
Contributor Author

/retest

@andyzhangx andyzhangx removed the request for review from edreed January 16, 2024 06:33
andyzhangx
andyzhangx reviewed Jan 16, 2024
View reviewed changes
pkg/azuredisk/azure_controller_common.go
if err != nil {
return false, err
}
if _, err := diskClient.Get(ctx, resourceGroup, diskName); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

why not add subsID in DisksClient.Get func? there would be easier to use in csi driver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I agree. but the subid is part of client object. https://github.com/Azure/azure-sdk-for-go/blob/775c7070836e584f796bc72c6de5c77649a67bb2/sdk/resourcemanager/compute/armcompute/disks_client.go#L27

Copy link
Member

Choose a reason for hiding this comment

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

could you write a wrapper func in this driver for this? I see this code is duplicated multiple times in this PR.

diskClient, err := c.clientFactory.GetDiskClientForSub(subsID)
if err != nil {
    return false, err
}
if _, err := diskClient.Get(ctx, resourceGroup, diskName); err != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is necessary because different method is invoked (get/delete/list)

Copy link
Member

Choose a reason for hiding this comment

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

this diskClient.Get func is called 13 times in this PR, why not wrap as a Driver func, that could reduce duplicated code a lot

Copy link
Contributor Author

@MartinForReal MartinForReal Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

diskClient.Delete is also invoked , diskclient can't be shared in one function if we put diskclient into wrapper.

@MartinForReal MartinForReal changed the title Refactor: migrate disk client to track2 sdk [wip]Refactor: migrate disk client to track2 sdk Jan 16, 2024
@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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 16, 2024
@MartinForReal MartinForReal changed the title [wip]Refactor: migrate disk client to track2 sdk Refactor: migrate disk client to track2 sdk Jan 17, 2024
@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 Jan 17, 2024
@MartinForReal
migrate disk client to track2 sdk
b00c447 
Signed-off-by: Fan Shang Xiang <shafan@microsoft.com>
@MartinForReal
Copy link
Contributor Author

/retest

5 similar comments
@MartinForReal
Copy link
Contributor Author

/retest

@MartinForReal
Copy link
Contributor Author

/retest

@MartinForReal
Copy link
Contributor Author

/retest

@MartinForReal
Copy link
Contributor Author

/retest

@MartinForReal
Copy link
Contributor Author

/retest

andyzhangx
andyzhangx approved these changes Jan 18, 2024
View reviewed changes
Copy link
Member

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, MartinForReal

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 lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 18, 2024
@MartinForReal
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit ba7f97a into kubernetes-sigs:master Jan 19, 2024
26 checks passed
@MartinForReal MartinForReal deleted the shafan/track2 branch January 19, 2024 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyzhangx andyzhangx andyzhangx approved these changes

@ZeroMagic ZeroMagic Awaiting requested review from ZeroMagic

@cvvz cvvz Awaiting requested review from cvvz

Assignees

@andyzhangx andyzhangx

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MartinForReal @k8s-ci-robot @andyzhangx