-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Block update change to labels mode #8927
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 @Tratcher, |
|
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 prevents changing the Container Apps revisions mode to labels via az containerapp update or az containerapp up, and instead directs users to the dedicated az containerapp revision set-mode command.
- Block mode-switch logic in the decorator for
labelsmode - End-to-end tests for both
updateandupcommands expecting the new error - HISTORY updated to note the disallowed change
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/containerapp/azext_containerapp/tests/latest/test_containerapp_commands.py | Added test_containerapp_update_mode_e2e to assert an error is thrown when using --revisions-mode labels with update/up |
| src/containerapp/azext_containerapp/containerapp_decorator.py | Introduced a guard that checks for existing labels mode and raises an error directing users to revision set-mode |
| src/containerapp/HISTORY.rst | Documented the disallowed --revisions-mode labels change in the upcoming release notes |
Comments suppressed due to low confidence (5)
src/containerapp/azext_containerapp/tests/latest/test_containerapp_commands.py:1692
- [nitpick] The test name implies only
updatebut also coversup. Consider renaming totest_containerapp_update_and_up_labels_mode_e2efor clarity.
def test_containerapp_update_mode_e2e(self, resource_group):
src/containerapp/azext_containerapp/tests/latest/test_containerapp_commands.py:1709
- The test checks only the generic error text. It should also assert that the message includes the recommendation to use
az containerapp revision set-modeto ensure the guidance is accurate.
self.assertIn("is not in labels mode", str(e))
src/containerapp/azext_containerapp/containerapp_decorator.py:454
- [nitpick] The comment refers to "Set-Mode" generically. For consistency, update it to reference the full
az containerapp revision set-modecommand.
# Check if the app was previously in labels mode. If not, throw an error saying to use Set-Mode instead.
src/containerapp/HISTORY.rst:9
- [nitpick] In RST, command names are usually enclosed in double backticks. Consider changing
* 'az containerapp update': ...to*az containerapp update: ...for consistency.
* 'az containerapp update': Disallow changing --revisions-mode to Labels.
src/containerapp/HISTORY.rst:10
- [nitpick] Similarly, wrap
az containerapp upin double backticks to match project conventions:*az containerapp up: ....
* 'az containerapp up': Disallow changing --revisions-mode to Labels.
|
Hi @Tratcher Release SuggestionsModule: containerapp
Notes
|
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 |
|
Please take a look at this comment #8927 (comment) |
|
@zhoxing-ms what about it? There's already an entry in History and we're not planning a new release yet. |
|
+@Greedygre could you please review this PR as well? |
Co-authored-by: Yan Zhu <[email protected]>
|
|
||
| if safe_get(containerapp_def, "properties", "configuration", "activeRevisionsMode").lower() != "labels": | ||
| raise ArgumentUsageError( | ||
| "The containerapp '{}' is not in labels mode. Please use `az containerapp revision set-mode` to switch to labels mode first.".format( |
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.
later we can remove "labels" value for "containerapp update" and "up" command.
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.
Good point, I had added activeRevisionsMode to up and update before I knew about set-mode, but we could remove them. The tricky part for up is that we do want to be able to create an app in labels mode.
In theory we could replicate the logic from set-mode here, but I'd rather not, switching modes is not a common operation.
…o user/chrross/nochangelabels # Conflicts: # src/containerapp/HISTORY.rst
Co-authored-by: Greedygre <[email protected]>
|
Hi @Juliehzl |
Changing revisions-mode from single to labels requires changes to the traffic section at the same time. There's a dedicated command that handles this,
az containerapp revision set-mode, so I've added an error message to update and up to redirect people there.Fixes https://msazure.visualstudio.com/Antares/_workitems/edit/33230254
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
az containerapp update --revisions-mode
az containerapp up --revisions-mode
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)