-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[App Service] Easy Auth V2 Commands #3502
Conversation
App Service |
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.
A few commands and questions based on my review of the samples.
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.
Overall I think it looks pretty good! Mostly some nitpicks and logistics questions.
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.
A few more nits/questions, but I don't see anything fundamentally blocking. Great work on this!
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.
LGTM!
@Juliehzl @zhoxing-ms The only CI checks that are failing are the integrated tests for other extensions (not anything I touched). Is there any way to get passed those blockers if we want to release this extension soon? |
Hi @mkarmark, please merge or rebase your PR with latest main branch. |
#sign-off |
@Juliehzl That worked, thanks! What steps are left for me to merge this PR? |
Please resolve conflicts to make CI pass. service name items need to be in alphabetical order |
@@ -0,0 +1,3 @@ | |||
{ |
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.
what is the reason for this? This will override all the webapp extension commands to not have preview tag - & that is not correct. Please verify & remove.
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.
so this is a separate extension from the webapps extension and these commands are not preview, so I'm confused how it would override a diff extension commands?
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.
Few comments to address.
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.
A few minor comments. otherwise looks good.
@Juliehzl @panchagnula my local test generates a recording that uses 2020-10-01 but the test expects api version 2021-04-01. Do you know what I can do to mitigate this error? |
@panchagnula could you please help above question since Zunli is OOF? |
@mkarmark as we discussed pleas wait for @calvinsID to update the CLI extension to use the track 2 SDK version & then re-base. Thanks! |
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.
LGTM
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
azdev style <YOUR_EXT>
locally? (pip install azdev
required)python scripts/ci/test_index.py -q
locally?For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update
src/index.json
automatically.The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify
src/index.json
.