-
Notifications
You must be signed in to change notification settings - Fork 1.5k
{AKS} Vendor new SDK and bump API version to 2025-04-02-preview #8837
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
️✔️Azure CLI Extensions Breaking Change Test
|
|
Hi @FumingZhang, |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
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.
Pull Request Overview
This PR vendors a new SDK and updates the API version from 2025-03-02-preview to 2025-04-02-preview. The key changes include updating all recorded URIs in test recordings, modifying the SDKProfile in the extension’s initialization, and updating release documentation in HISTORY.rst and README.rst.
Reviewed Changes
Copilot reviewed 262 out of 262 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/recordings/*.yaml | Updated API version URIs for consistent versioning in tests |
| managed_cluster_decorator.py | Commented-out safeguards code now has a TODO for the new deployment safeguard API |
| init.py | Updated SDKProfile API version to 2025-04-02-preview |
| README.rst and HISTORY.rst | Updated release notes and versioning details to reflect the new API version |
| # TODO (NickKeller): update this according to the new deployment safeguard API | ||
| # excludedNamespaces = self.context.get_safeguards_excluded_namespaces() | ||
| # version = self.context.get_safeguards_version() | ||
| # level = self.context.get_safeguards_level() | ||
| # # provided any value? | ||
| # mc = setup_common_safeguards_profile(level, version, excludedNamespaces, mc, self.models) |
Copilot
AI
Jun 6, 2025
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.
Consider removing or cleaning up the commented-out safeguards profile code in set_up_safeguards_profile once the new deployment safeguard API is available, or reference a tracking issue to avoid leaving dead code in production.
| # TODO (NickKeller): update this according to the new deployment safeguard API | |
| # excludedNamespaces = self.context.get_safeguards_excluded_namespaces() | |
| # version = self.context.get_safeguards_version() | |
| # level = self.context.get_safeguards_level() | |
| # # provided any value? | |
| # mc = setup_common_safeguards_profile(level, version, excludedNamespaces, mc, self.models) | |
| # TODO (NickKeller): Update this method when the new deployment safeguard API becomes available. | |
| # Tracking issue: https://github.com/Azure/azure-cli/issues/12345 |
| """Update safeguards profile for the ManagedCluster object | ||
| :return: the ManagedCluster object | ||
| """ | ||
| # TODO (NickKeller): update this according to the new deployment safeguard API | ||
|
|
||
| self._ensure_mc(mc) | ||
| # self._ensure_mc(mc) | ||
|
|
||
| excludedNamespaces = self.context.get_safeguards_excluded_namespaces() | ||
| version = self.context.get_safeguards_version() | ||
| level = self.context.get_safeguards_level() | ||
| # excludedNamespaces = self.context.get_safeguards_excluded_namespaces() | ||
| # version = self.context.get_safeguards_version() | ||
| # level = self.context.get_safeguards_level() | ||
|
|
||
| mc = setup_common_safeguards_profile(level, version, excludedNamespaces, mc, self.models) | ||
| # mc = setup_common_safeguards_profile(level, version, excludedNamespaces, mc, self.models) | ||
|
|
||
| if level is not None: | ||
| mc.safeguards_profile.level = level | ||
| if version is not None: | ||
| mc.safeguards_profile.version = version | ||
| # if level is not None: | ||
| # mc.safeguards_profile.level = level | ||
| # if version is not None: | ||
| # mc.safeguards_profile.version = version | ||
|
|
Copilot
AI
Jun 6, 2025
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 commented-out code in update_safeguards_profile (including the _ensure_mc call and safeguards configuration) could be removed or clearly marked with a reference to an issue, which would improve code clarity and maintainability.
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 whether these commented-out lines of code have any impact on the existing logic?
|
| # version = self.context.get_safeguards_version() | ||
| # level = self.context.get_safeguards_level() | ||
| # # provided any value? | ||
| # mc = setup_common_safeguards_profile(level, version, excludedNamespaces, mc, self.models) |
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 whether these commented-out lines of code have any impact on the existing logic?
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 options related to the 'safe guard' feature are being updated. A new backend API design has been introduced, and the feature owner has removed the current safe guard profile from the AKS Swagger. A separate profile will be added soon and feature owner add new implementation then.
For now, as the safe guard related model has been removed, I have to comment out the implementation to pass the test.
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.
+ feature owner @NickKeller to help take another look
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.
Any code that sets mc.safeguards_profile is fine to delete instead of comment out. I'd like to leave in the cli flags though, as I want to use those as syntactic sugar to create our resource
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.
OK, I'll remove the implementations
| c.argument("safeguards_excluded_ns", type=str, is_preview=True) | ||
| c.argument( | ||
| "safeguards_excluded_ns", | ||
| deprecate_info=c.deprecate( |
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.
I can always "un-deprecate" this option when my PR merges and the sdk for my new resource is generated?
It's not going to add to the managed cluster model, but I would like to use it to specify creating our resource instead
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.
I think "un-deprecate" is not acceptable, I'll revert these changes and let you decide whether to deprecate them.
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
[Release] Update index.json for extension [ aks-preview-18.0.0b11 ] : https://dev.azure.com/msazure/One/_build/results?buildId=126776792&view=results |
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
--safeguards-level,--safeguards-versionand--safeguards-excluded-nsinaz aks createandaz aks updatecommands.General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)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.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.