Repository navigation
[ACR] BREAKING CHANGE: connected-registry: commands update - #28682
Rodrigo Mendoza (rosanch) wants to merge 10 commits into
Conversation
❌AzureCLI-FullTest
|
❌AzureCLI-BreakingChangeTest
|
|
ACR |
microsoft-github-policy-service
Bot
added
Auto-Assign
|
Xing Zhou (@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? |
|
Xing Zhou (@zhoxing-ms) any updates regarding the testing issue?? |
|
Rodrigo Mendoza (@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) |
|
Xing Zhou (@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. |
Xing Zhou (zhoxing-ms)
changed the title
Xing Zhou (zhoxing-ms)
changed the title
|
Rodrigo Mendoza (@rosanch) Huangli Wu (@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) |
Xing Zhou (@zhoxing-ms), the failure seems unrelated to the change. Can we rerun the tests? |
|
Huangli Wu (@huanwu) Rodrigo Mendoza (@rosanch) The reason for these CI issues is that the current yaml file has one less GET request than the code logic, which may be due to a code logic change without re-recording the yaml file. Could you help re-record these failed tests |
|
Rodrigo Mendoza (@rosanch) Huangli Wu (@huanwu) By the way, please add some new test cases for this PR change |
| 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...
Xing Zhou (zhoxing-ms)
modified the milestones:
April 2024 (2024-04-30),
May 2024 (2024-05-21)❗ Breaking change release ❗
|
Rodrigo Mendoza (@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 |
Xing Zhou (zhoxing-ms)
modified the milestones:
May 2024 (2024-05-21)❗ Breaking change release ❗,
June 2024 (2024-07-02)
|
Rodrigo Mendoza (@rosanch) Please resolve these conflicts and CI issues |
|
#28682 (comment) Please address this comment |
Xing Zhou (zhoxing-ms)
modified the milestones:
June 2024 (2024-07-09),
July 2024 (2024-08-06)
|
Rodrigo Mendoza (@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 |
|
Rodrigo Mendoza (@rosanch) Gavin King (@getk12) Is this still needed |
|
Rodrigo Mendoza (@rosanch) Gavin King (@getk12) – Moving this back to Draft for now since this PR has been stale for quite some time and hasn’t seen any recent updates. |
This branch has not been deployed
11 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.