Skip to content

{testsdk} Remove CommunicationResourcePreparer from azure-cli-testsdk #20717

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.

Sign up for GitHub

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

Jump to bottom
Merged
jiasli merged 4 commits into from
Dec 28, 2021
Merged

{testsdk} Remove CommunicationResourcePreparer from azure-cli-testsdk #20717

jiasli merged 4 commits into from
Dec 28, 2021

Conversation

@moirf
Copy link

@moirf moirf commented Dec 13, 2021

Description

Add default connection string required in communication extension in resource preparer file.
Testing Guide

NA
History Notes

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

@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Dec 13, 2021
@ghost
Copy link

ghost commented Dec 13, 2021

Thank you for your contribution moirf! We will review the pull request and get back to you soon.

beltr0n
beltr0n reviewed Dec 13, 2021
View reviewed changes
Copy link
Member

@beltr0n beltr0n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit comment about consistency but code looks good.
Please update the PR description because that is causing the pipeline validation to fail

@wangzelin007 wangzelin007 changed the title Add default connection string required in communication extension {testsdk} Add default connection string required in communication extension Dec 13, 2021
@wangzelin007
Copy link
Member

wangzelin007 commented Dec 13, 2021

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 13, 2021

Azure Pipelines successfully started running 2 pipeline(s).

jiasli
jiasli requested changes Dec 14, 2021
View reviewed changes
Copy link
Member

@jiasli jiasli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add more context for why this is changed.

@v-mausha @beltr0n
Update src/azure-cli-testsdk/azure/cli/testsdk/preparers.py
47dc7a2 
Used in azext_communication extension for sanitization in playback test mode.

Co-authored-by: Bert Ong <[email protected]>
@moirf moirf requested a review from jiasli December 14, 2021 05:57
@jiasli jiasli self-assigned this Dec 15, 2021
@moirf
Copy link
Author

moirf commented Dec 15, 2021

Please add more context for why this is changed.
The test cases for azure-cli-extension, validate the endpoints, for consistency we update the endpoint matching with sanitized.

@beltr0n
Copy link
Member

beltr0n commented Dec 15, 2021 •
edited
Loading

I think I'd also like to know, did the previous value work, or is this just a change for consistency? This isn't a new value being added, it's a change to an existing value. I believe for Azure SDK, there's code that checks the value conforms to a certain format.
Also, can you add the context to the description?

@moirf
Copy link
Author

moirf commented Dec 17, 2021 •
edited
Loading

@beltr0n @jiasli we are writing the cli-extension commands for communication, the PR for this is pending here - Azure/azure-cli-extensions#4206
For running the live mode, the test cases are passed because the account_key( which is actually connection string see line 185) will return the string into correct format, in case of playback mode also we need it as connection string format. since this is not into a correct connection string format, test cases failed as it is looking for an URI into it.
Previous value didn't work, it was a mistake in last commit.

@jiasli
Copy link
Member

jiasli commented Dec 21, 2021 •
edited
Loading

I would suggest not to put CommunicationResourcePreparer (from #20552) in azure-cli-testsdk, as azure-cli-testsdk is only for common preparers. Instead, it should go to your own command module/extension.

@yonzhan yonzhan added this to the Backlog milestone Dec 21, 2021
@moirf
Copy link
Author

moirf commented Dec 21, 2021

@jiasli Ok, will revert the changes for #20552. And close this PR.

@jiasli jiasli changed the title {testsdk} Add default connection string required in communication extension {testsdk} Remove CommunicationResourcePreparer from azure-cli-testsdk Dec 24, 2021
@moirf
Copy link
Author

moirf commented Dec 27, 2021

@yonzhan @jiasli can you please merge the branch as I do not have access.

@jiasli jiasli merged commit 99d7a01 into Azure:dev Dec 28, 2021
@jiasli
Copy link
Member

jiasli commented Dec 28, 2021

Done. Thanks for notifying me.

@jiasli jiasli modified the milestones: Backlog, Dec 2021 (2022-01-04) Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jiasli jiasli jiasli approved these changes

@wangzelin007 wangzelin007 wangzelin007 approved these changes

@yonzhan yonzhan yonzhan approved these changes

@jsntcy jsntcy Awaiting requested review from jsntcy jsntcy is a code owner

@kairu-ms kairu-ms Awaiting requested review from kairu-ms kairu-ms is a code owner

+1 more reviewer

@beltr0n beltr0n beltr0n approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jiasli jiasli

Labels

customer-reported Issues that are reported by GitHub users external to the Azure organization.

Projects

None yet

Milestone

Dec 2021 (2022-01-04)

Development

Successfully merging this pull request may close these issues.

6 participants

@moirf @wangzelin007 @beltr0n @jiasli @yonzhan @v-mausha