-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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-preview: feature support for web application routing #4872
aks-preview: feature support for web application routing #4872
Conversation
…e-resource-id parameter.
…uting --dns-zone-resource-id ..."
Fix the following warnings: E128 continuation line under-indented for visual indent W293 blank line contains whitespace E302 expected 2 blank lines, found 1
aks-preview |
Change c.argument('dns-zone-resource-id', options_list=['--dns-zone-resource-id']) to c.argument('dns-zone-resource-id') The options_list=['--dns-zone-resource-id'] just use the default option, so it is not needed to be specified.
…imilary to gitops addon: Run it in "set_up_addon_profiles".
…ebApplicationRouting" Since web application routing's settings are in its own IngressProfile, not addon profile, we don't need to defined this constant, which is used as the name in addon profile.
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 PR basically looks good to me. But I've to say there are three different mechanisms to enable an addon which is quite complicated and not all of them are covered by existing test cases.
src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py
Outdated
Show resolved
Hide resolved
src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py
Outdated
Show resolved
Hide resolved
I've made a note of this to add more test cases. Right now update test case can't be added yet because there's a bug on the backend for updating that's fixed last Friday. After that code is rolled out, I'll add the test cases. |
src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py
Outdated
Show resolved
Hide resolved
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
Co-authored-by: FumingZhang <[email protected]>
Co-authored-by: FumingZhang <[email protected]>
…or when addon is not enabled (thus the request isn't valid).
….com/yizhang4321/azure-cli-extensions into yizhang4321/AddWebApplicationRouting
…S backend. The bug has been fixed but the release hasn't been rolled out yet. I'll renable the check once the fix has been rolled out.
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style aks-preview
locally? (pip install azdev
required)Yes, I have run it. It showed many warnings, but not related to this PR
python scripts/ci/test_index.py -q
locally?Yes: result:
% python scripts/ci/test_index.py -q
Ran 9 tests in 0.045s
OK (skipped=2)
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.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
.