-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[AKS] Add --disable-http-proxy option #8815
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
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| aks update | cmd aks update added parameter disable_http_proxy |
|
Hi @lilypan26, |
|
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
Adds support for a new --disable-http-proxy flag to the az aks update command, enabling users to turn off HTTP proxy settings on an existing cluster.
- Introduced
disable_http_proxyCLI parameter and help text. - Implemented
get_disable_http_proxyandupdate_http_proxy_configin the update decorator. - Added unit and live tests to verify disabling the HTTP proxy.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/aks-preview/azext_aks_preview/_params.py | Registered new disable_http_proxy argument |
| src/aks-preview/azext_aks_preview/_help.py | Added help entry for --disable-http-proxy option |
| src/aks-preview/azext_aks_preview/managed_cluster_decorator.py | Implemented retrieval and handling of disable flag |
| src/aks-preview/azext_aks_preview/tests/latest/test_managed_cluster_decorator.py | Added unit tests for update_http_proxy_config |
| src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py | Added live CLI test for disabling proxy |
| src/aks-preview/HISTORY.rst | Updated changelog for new preview release |
Comments suppressed due to low confidence (2)
src/aks-preview/azext_aks_preview/tests/latest/test_managed_cluster_decorator.py:6499
- The test initializes
mc_2without anhttp_proxy_config, so disabling proxy will always hit the error path. Consider setting a defaulthttp_proxy_configonmc_2before callingupdate_http_proxy_config.
mc_2 = self.models.ManagedCluster(location="test_location")
src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py:6056
- [nitpick] The live CLI test only asserts the
enabledflag; consider adding checks to ensure existinghttpProxy,httpsProxy, andnoProxyvalues remain unchanged when disabling.
disable_cmd = "aks update --resource-group={resource_group} --name={name} --disable-http-proxy"
| if not mc.http_proxy_config: | ||
| raise UnknownError( | ||
| "Unexpectedly get an empty http proxy config in the process of updating http proxy enabled." | ||
| ) |
Copilot
AI
May 29, 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 check for not mc.http_proxy_config precedes the None initialization, so a null http_proxy_config always triggers an error and prevents creating a new config. Swap the order or remove the redundant error to allow initialization when missing.
| if not mc.http_proxy_config: | |
| raise UnknownError( | |
| "Unexpectedly get an empty http proxy config in the process of updating http proxy enabled." | |
| ) |
| ground_truth_mc_2 = self.models.ManagedCluster( | ||
| location="test_location", | ||
| http_proxy_config={ | ||
| "enabled": "false", |
Copilot
AI
May 29, 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 test expects the enabled field as the string "false", but the decorator sets it to the boolean False. Update the expected value to a boolean.
| "enabled": "false", | |
| "enabled": False, |
|
|
the change looks good to me, could you please fix failed CI checks, live test and resolve merge conflicts? |
90e4b2c to
6bc8769
Compare
|
[Release] Update index.json for extension [ aks-preview-18.0.0b7 ] : https://dev.azure.com/msazure/One/_build/results?buildId=126089759&view=results |
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
az aks update --disable-http-proxy
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.