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
appservice: support list publishing profiles #2504
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2504 +/- ##
==========================================
+ Coverage 72.45% 72.47% +0.02%
==========================================
Files 362 362
Lines 19550 19565 +15
Branches 2866 2869 +3
==========================================
+ Hits 14165 14180 +15
+ Misses 4474 4472 -2
- Partials 911 913 +2
Continue to review full report at Codecov.
|
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.
The PR looks good. We just need to have a discussion on how we move forward deprecating commands.
cli_command(__name__, 'appservice web deployment list-site-credentials', | ||
'azure.mgmt.web.operations.web_apps_operations#WebAppsOperations.list_publishing_credentials', factory) | ||
cli_command(__name__, 'appservice web deployment list-publishing-profiles', | ||
'azure.cli.command_modules.appservice.custom#list_publish_profiles') |
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.
Why are we removing a command (hence the breaking change)?
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.
It seems like this new command returns more things in a better format. Do we need to change the name? We should have a discussion on how to deprecate commands within the CLI. cc/ @johanste
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.
@tjprescott. Appservice command module is not GA yet, hence no deprecate is reuqired
publish_profiles
contains publishing credentials, so there are less reasons to expose both.
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.
Also publish_profiles
is a very well known terminology, so I suggest we should keep the name
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.
That makes sense. Does it show more information than the old command?
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.
Yes. it is a super set, and particularly, the FTP end points which users are asking about in #2399.
for f in content: | ||
full_xml += f.decode() | ||
|
||
profiles = xmltodict.parse(full_xml, xml_attribs=True)['publishData']['publishProfile'] |
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.
Do you want to check for key errors here or publishData
, publishProfile
will always exist?
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.
It should always be there. Since I worked on it from Visual Studio Tooling side several years ago, I have never seen the opposite :). So i suggest we keep the assumption. The worst case is we see a bad error and we should figure out why either way at that time.
Fix #2399
This command will convert xml to json so users can user jmespath query to retrieve all they want. For example,
az appservice web deployment list-publishing-profiles --query "[?publishMethod=='FTP'].publishUrl" -otsv
would return the ftp endpoint