[aks-preview] Fix operator precedence bug in get_container_network_logs() validation#9575
Conversation
️✔️Azure CLI Extensions Breaking Change Test
|
|
Hi @ShantingLiu, |
|
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>
|
CodeGen Tools Feedback CollectionThank you for using our CodeGen tool. We value your feedback, and we would like to know how we can improve our product. Please take a few minutes to fill our codegen survey |
The validation logic had two issues: 1. Operator precedence bug - the condition evaluated as (enable_cnl AND acns_not_enabled) OR monitoring_not_enabled which would error if monitoring wasn't configured, regardless of enable_cnl 2. Timing issue during cluster create - the validation only checked mc.addon_profiles, but during create this is empty even when --enable-addons monitoring is specified Fix: Check both mc.addon_profiles (for update) AND --enable-addons parameter (for create), with correct operator precedence.
e8747b5 to
aee6e23
Compare
|
Hi @ShantingLiu Release SuggestionsModule: aks-preview
Notes
|
There was a problem hiding this comment.
Pull request overview
Fixes validation in get_container_network_logs() to avoid incorrect failures caused by operator precedence and to correctly detect monitoring enablement during cluster create.
Changes:
- Restructures validation so dependency checks only run when container network logs are being enabled.
- Adds create-time detection for monitoring enablement via the
--enable-addonsparameter in addition to existingmc.addon_profileschecks.
Address Copilot feedback: improve monitoring enablement detection - Use normalized addon list via _get_enable_addons() when available - Handle both string and list formats for enable_addons parameter - Also check --enable-azure-monitor-logs flag as alternative way to enable monitoring - Use CONST_MONITORING_ADDON_NAME_CAMELCASE for proper constant matching Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ADDON_NAME_CAMELCASE The enable_addons parameter contains the user-facing addon name 'monitoring', not the profile key name 'omsAgent' (CONST_MONITORING_ADDON_NAME_CAMELCASE). This caused the check to always fail since 'omsAgent' is never in the enable_addons list.
| "Container network logs requires '--enable-acns', advanced networking " | ||
| "to be enabled, and the monitoring addon to be enabled." | ||
|
|
||
| if enable_cnl: |
The validation logic in
get_container_network_logs()had two issues:Fix: Check both mc.addon_profiles (for update) AND --enable-addons parameter (for create), with correct operator precedence.
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
az aks create --enable-container-network-logsaz aks update --enable-container-network-logsGeneral 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.