-
Notifications
You must be signed in to change notification settings - Fork 84
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
[release-1.23] feat: support workload identity setting in static PV mount #1211
[release-1.23] feat: support workload identity setting in static PV mount #1211
Conversation
Signed-off-by: Fan Shang Xiang <shafan@microsoft.com>
Signed-off-by: Fan Shang Xiang <shafan@microsoft.com>
…s-sigs#1204) * feat: support workload identity setting in static PV mount * fix
fcdfecf
to
78b9a9e
Compare
/retest |
1 similar comment
/retest |
@@ -164,18 +165,6 @@ func TestCreateVolume(t *testing.T) { | |||
name string | |||
testFunc func(t *testing.T) | |||
}{ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this ut is removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deleted uts are cherry-picked from ad9d27c#diff-0b174fc042381a284b07f142da447934352f84b42ab6fb5c3cc5082857b376b5
@@ -1321,21 +1296,6 @@ func TestControllerExpandVolume(t *testing.T) { | |||
} | |||
}, | |||
}, | |||
{ | |||
name: "invalid expand volume req", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this ut is removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
/retest |
1 similar comment
/retest |
/retest |
@andyzhangx Could you please take another look? The unit test coverage decreased was caused by the cherry-pick, . And I've validated that the basic function is normal. |
@cvvz could you add ut back in master branch in another PR? I think we need to add those ut back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvvz could you add ut back in master branch in another PR? I think we need to add those ut back.
cc @MartinForReal
Yes, That's what I'm thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx, cvvz 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 |
4183155
into
kubernetes-sigs:release-1.23
@andyzhangx I've checked the code refactor and these ut are ok to be deleted, it won't affect the coverage rate. Actually, there is another ut that din't cover any code, which decrease the coverage rate. I've filed a PR #1214 to fix it. |
This is a cherry-pick of #1204
Here are the csi driver's logs:
NodeStageVolume
, then we use service account token to get the account key duringNodePublishVolume
NodePublishVolume