[ACR] BREAKING CHANGE: connected-registry: commands update#28682
[ACR] BREAKING CHANGE: connected-registry: commands update#28682
Conversation
❌AzureCLI-FullTest
|
❌AzureCLI-BreakingChangeTest
|
|
ACR |
microsoft-github-policy-service
bot
requested review from
jiasli,
wangzelin007 and
yonzhan
microsoft-github-policy-service
bot
added
Auto-Assign
|
@zhoxing-ms I see coverage failing saying we don't have tests for acr connected-registry update but we do. Can you please take a look? |
src/azure-cli/azure/cli/command_modules/acr/connected_registry.py
Outdated
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/acr/connected_registry.py
Outdated
Show resolved
Hide resolved
src/azure-cli/azure/cli/command_modules/acr/connected_registry.py
Outdated
Show resolved
Hide resolved
|
@zhoxing-ms any updates regarding the testing issue?? |
|
@rosanch Please resolve these CI issues. Please note that we are launching the release for this sprint this week. Please resolve all comments by tomorrow, otherwise the release of this PR will have to be postponed to the next sprint (on 05-21) |
|
@zhoxing-ms The issue states that we do not have a az connected-registry update test. However, we do. I believe the testing must be confused because we changed the command from using the custom_command to the generic_update_command to follow CLI best practices. However, the PR testing is saying we do not have any tests in place for the command. |
|
@zhoxing-ms , this is the test for az acr connected-registry update https://github.com/Azure/azure-cli/blob/dev/src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_connectedregistry_commands.py#L101 for |
zhoxing-ms
changed the title
zhoxing-ms
changed the title
|
@rosanch @huanwu Actually, I saw other errors in the CI pipeline pipeline link . Please resolve these CI errors first |
Additionally, as this PR includes some breaking changes, it can usually only be released in our breaking change window (Ignite or Build Sprint). Therefore, I suggest postponing its release to the next sprint as the next sprint is a breaking change window for Build (05-21) |
@zhoxing-ms, the failure seems unrelated to the change. Can we rerun the tests? |
| with self.argument_context('acr connected-registry repo') as c: | ||
| c.argument('add_repos', options_list=['--add'], nargs='*', | ||
| help='repository permissions to be added to the targeted connected registry and it\'s ancestors sync scope maps. Use the format "--add [REPO1 REPO2 ...]" per flag. ' + repo_valid_actions) | ||
| c.argument('remove_repos', options_list=['--remove'], nargs='*', | ||
| help='repository permissions to be removed from the targeted connected registry and it\'s succesors sync scope maps. Use the format "--remove [REPO1 REPO2 ...]" per flag. ' + repo_valid_actions) |
There was a problem hiding this comment.
Please add deprecate_info first to deprecate them, and then remove them in the breaking change window of next sprint
There was a problem hiding this comment.
The entire command was already removed.
There was a problem hiding this comment.
What I mean is that these commands are more recommended to be removed in the breaking change window of the next sprint, otherwise it may cause unexpected damage to the customers' automation scripts. You can read this guideline breaking_changes to learn more about CLI breaking change process
There was a problem hiding this comment.
thank you I'll remove them. I wasn't aware of this new policy. I thought it'd be fine for this command since they were only available for the month of releasee of the feature 2 years ago.
| acr connected-registry update: | ||
| rule_exclusions: | ||
| - missing_command_test_coverage |
There was a problem hiding this comment.
Please add some test cases for this command instead of excluding it in tests
There was a problem hiding this comment.
I already did. That's what I've been asking about for a weeks now. The testing is not reacting to it.
There was a problem hiding this comment.
Check the test file. The update test has been there for years now...
zhoxing-ms
modified the milestones:
April 2024 (2024-04-30),
May 2024 (2024-05-21)❗ Breaking change release ❗
|
@rosanch Could you please resolve this conflict file? |
|
Please note that we are launching the release for this sprint this week. Please resolve all comments by tomorrow, otherwise the release of this PR will have to be postponed to the next sprint (on 07-02) |
|
please resolve CI issues |
zhoxing-ms
modified the milestones:
May 2024 (2024-05-21)❗ Breaking change release ❗,
June 2024 (2024-07-02)
|
@rosanch Please resolve these conflicts and CI issues |
|
#28682 (comment) Please address this comment |
|
@rosanch Since we haven't received a response to this PR for a long time, I will put it in the backlog first. After you have resolved its comments and conflicts, we will consider adding it to the release milestone |
10 participants
Related command
az acr connected-registry create/update/install
Description
Minor updates on ACR connected registry commands:
Testing Guide
1.1 respond no
1.2 try again respond yes
History Notes
[ACR] BREAKING CHANGE:
az acr connected-registry install: Group and sub-commands removed[ACR] BREAKING CHANGE:
az acr connected-registry create: Mode default value change from ReadWrite to ReadOnly[ACR]
az acr connected-registry create: If data-endpoint disabled ask for confirmation to enable it instead of throwing an errorThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.