Skip to content

Conversation

@savaradh
Copy link
Member

@savaradh savaradh commented Dec 17, 2021

Description
Add notifications list to acr connected-registry create and update commands

History Notes
[ACR] az acr connected-registry create: Add --notifications to support adding patterns for generating notification events on connected registry artifacts
[ACR] az acr connected-registry update: Add --add-notifications and --remove-notifications to support adding or removing patterns for generating notification events on connected registry artifacts


This checklist is used to make sure that common guidelines for a pull request are followed.

Add notifications list to acr connected-registry create and update
commands
@savaradh
Copy link
Member Author

@rosanch @northtyphoon

@savaradh
Copy link
Member Author

@zhoxing-ms Can you please help me with this? I don't understand the reason for build failure. I created live recording and pushed them. However I am noticing the failures

@yonzhan yonzhan added this to the Jan 2022 (2022-02-01) milestone Dec 23, 2021
@yonzhan yonzhan requested a review from wangzelin007 December 23, 2021 07:57
@wangzelin007 wangzelin007 changed the title [Acr] az acr connected-registry: Add --notifications-list argument [Acr] az acr connected-registry: Add --notifications-list argument Dec 24, 2021
@zhoxing-ms zhoxing-ms changed the title [Acr] az acr connected-registry: Add --notifications-list argument [ACR] az acr connected-registry: Add --notifications-list argument to support the list of artifact pattern for which notifications need to be generated Dec 27, 2021
@yonzhan
Copy link
Collaborator

yonzhan commented Dec 27, 2021

ACR

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Dec 29, 2021

@savaradh Any update? Please note that we will launch the release of this sprint the day after tomorrow. If you can't address these comments tomorrow, this PR will not catch up with the release of this sprint and can only be postponed to the next sprint (02-08)

@savaradh
Copy link
Member Author

savaradh commented Dec 30, 2021

@zhoxing-ms Thank you. I will fix the tests and push it today. I would like to get it on today's release

@savaradh savaradh changed the title [ACR] az acr connected-registry: Add --notifications-list argument to support the list of artifact pattern for which notifications need to be generated [ACR] az acr connected-registry: Add --notifications argument to support the list of artifact pattern for which notifications need to be generated Dec 31, 2021
@zhoxing-ms
Copy link
Contributor

@savaradh Sorry, because the comments were resolved a little late, and included upgrading api-version, this poses a risk to the quality of CLI and may blocking CLI release.
Therefore, this PR cannot catch up with the release of this sprint. Next time, please try to get the PR ready on the code completion date. The release time of the next sprint is 2022-02-08 (this is also the milestone we set at the beginning for this PR)

@zhoxing-ms
Copy link
Contributor

        except CannotOverwriteExistingCassetteException as ex:
>           raise AssertionError(ex)
E           AssertionError: Can't overwrite existing cassette ('/home/vsts/work/1/s/src/azure-cli/azure/cli/command_modules/acr/tests/latest/recordings/test_acr_image_import.yaml') in your current record mode ('once').
E           No match for the request (<Request (PUT) https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/resourcegroupsamesub/providers/Microsoft.ContainerRegistry/registries/sourceregistrysamesub000002?api-version=2021-08-01-preview>) was found.
E           Found 1 similar requests with 1 different matcher(s) :
E           
E           1 - (<Request (PUT) https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/resourcegroupsamesub/providers/Microsoft.ContainerRegistry/registries/sourceregistrysamesub000002?api-version=2021-06-01-preview>).
E           Matchers succeeded : ['method', 'scheme', 'host', 'port', 'path']
E           Matchers failed :
E           _custom_request_query_matcher - assertion failure :
E           None

src/azure-cli-testsdk/azure/cli/testsdk/base.py:299: AssertionError
- generated xml file: /home/vsts/.azdev/env_config/home/vsts/work/1/s/env/test_results.xml -
=========================== short test summary info ============================
FAILED src/azure-cli/azure/cli/command_modules/acr/tests/latest/test_acr_commands.py::AcrCommandsTests::test_acr_image_import
=========== 1 failed, 2703 passed, 316 skipped in 3344.84s (0:55:44) ===========

Please re-run the test_acr_image_import test in live mode and re-record the yaml file for this test to solve the CI issue

@savaradh
Copy link
Member Author

@zhoxing-ms no problem. The PR was delayed cause of the holidays. Please keep me posted if there is any change in the release dates. I am still trying to fix one test which I am unable to record in live mode. It is skipped when I run locally and I reached out to my team to help fix it. We can merge the PR once I fix it.

@savaradh
Copy link
Member Author

The test is skipped when I try to run on live mode. I notice the annotation @record_only() for that particular test.

@zhoxing-ms
Copy link
Contributor

Yes, the tests annotated with @record_only() cannot be executed in live mode.
You can try to delete the @record_only() annotation of this test. If this is not possible, please manually replace the inconsistent api-version in the test_acr_image_import.yaml file

@savaradh
Copy link
Member Author

savaradh commented Jan 3, 2022

Thank you for the details @zhoxing-ms . I manually updated the api version on the recording.

@savaradh
Copy link
Member Author

savaradh commented Jan 3, 2022

@jsntcy @kairu-ms @wangzelin007 Can one of you review this change so that I can get it merged? Thanks

@savaradh
Copy link
Member Author

savaradh commented Jan 5, 2022

@zhoxing-ms Can you please merge this?

@rosanch
Copy link
Contributor

rosanch commented Jan 6, 2022

@savaradh can you please fix the history release notes and request @toddysm to review them?
You can use this PR as an example #17566

@savaradh
Copy link
Member Author

savaradh commented Jan 6, 2022

@savaradh can you please fix the history release notes and request @toddysm to review them? You can use this PR as an example #17566

Thanks @rosanch. I updated the history notes. @toddysm Can you please check if it looks good?

@toddysm
Copy link
Member

toddysm commented Jan 10, 2022

LGTM

@savaradh
Copy link
Member Author

@zhoxing-ms Can you please merge this? The history notes has been approved

@zhoxing-ms zhoxing-ms merged commit 80cf4d4 into Azure:dev Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants