-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[AKS]: Fix Scenario test failure for Azure Container Storage #7512
Conversation
️✔️Azure CLI Extensions Breaking Change Test
|
Hi @mukhoakash, |
AKS |
Live test for the failing test case: test_aks_update_with_azurecontainerstorage https://dev.azure.com/msazure/CloudNativeCompute/_build/results?buildId=91616856&view=results |
|
@@ -4934,79 +4934,6 @@ def postprocessing_after_mc_created(self, cluster: ManagedCluster) -> None: | |||
:return: None | |||
""" | |||
super().postprocessing_after_mc_created(cluster) | |||
enable_azure_container_storage = self.context.get_intermediate("enable_azure_container_storage") |
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.
Glad to see the live test passed, but I have some concerns about this change.
Users may not necessarily use the version of azure-cli (2.59.0+) that contains the changes in #28251. For example, if the user installs azure-cli 2.58.0 and the latest version of aks-preview that contains the current changes, then these enable/disable operations will not be performed, which is equivalent to the container storage related options being ignored, and this is unexpected.
File azext_metadata.json is used to restrict aks-preview's dependence on the version of azure-cli, which is currently 2.56.0. And we don’t want to bump it yet, because this will make the restrictions too strict.
The most ideal solution is that the operation can be implemented as idempotent. If it is difficult to achieve the former, is it possible to do some checks before starting these operations in aks-preview? If these operations have already been done, skip them, otherwise execute them.
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.
Ensured that there is no depedency on az cli version in the latest set of changes.
This reverts commit 7c74ba6.
Live test with the latest changes.
|
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
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.
Need support
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 azdev
required)python scripts/ci/test_index.py -q
locally? (pip install wheel==0.30.0
required)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.json
automatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json
.