-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[AKS] Prompt when disabling CSI Drivers #4868
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
Conversation
|
AKS |
|
Please address those conflicts and the 7 failing checks. |
939ca17 to
911752e
Compare
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.
please make sure there are no existing Kubernetes resources that are used by the CSI driver before disabling
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.
@olsenme are you ok with the above statement in the cli command?
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.
Changed.
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.
please make sure there are no existing Kubernetes resources that are used by the snapshot controller before disabling
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.
@olsenme are you ok with the above statement in the cli command?
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.
Changed.
ce74139 to
0bccf1f
Compare
93ce389 to
1676ce1
Compare
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.
Please put these statements below raw_parameters = locals(). Also put them into decorator.py? It's hard to test the entry function (aks_create).
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.
done
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.
Similar to above.
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.
done
1676ce1 to
d534ce0
Compare
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 prompt may happen in az aks create command? it should only prompt in az aks update
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.
Make sense. Let me add it into self.decorator_mode == DecoratorMode.UPDATE
d534ce0 to
de7b30b
Compare
cf76919 to
20ff17e
Compare
Signed-off-by: Ji An Liu <[email protected]>
20ff17e to
4a5fb99
Compare
FumingZhang
left a comment
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
| msg = "Please make sure there are no existing PVs and PVCs that are used by AzureDisk CSI driver before disabling." | ||
| if not self.get_yes() and not prompt_y_n(msg, default="n"): | ||
| raise DecoratorEarlyExitException() |
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.
May I ask will the execution of CLI scripts in the automation scenario be blocked by such interactive steps that require input?
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.
yes. but -y can be added into scripts like what we do in test_aks_command.py
| msg = "Please make sure there are no existing PVs and PVCs that are used by AzureFile CSI driver before disabling." | ||
| if not self.get_yes() and not prompt_y_n(msg, default="n"): | ||
| raise DecoratorEarlyExitException() |
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.
Same as above
| msg = "Please make sure there are no existing VolumeSnapshots, VolumeSnapshotClasses and VolumeSnapshotContents " \ | ||
| "that are used by the snapshot controller before disabling." | ||
| if not self.get_yes() and not prompt_y_n(msg, default="n"): | ||
| raise DecoratorEarlyExitException() |
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.
Same as above
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally?For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify
src/index.json.