Skip to content

Use the app service plan's resource group for cert operations#2755

Merged
yugangw-msft merged 1 commit intoAzure:masterfrom
phekmat:bugfix/web_app_ssl_resource_group
Apr 10, 2017
Merged

Use the app service plan's resource group for cert operations#2755
yugangw-msft merged 1 commit intoAzure:masterfrom
phekmat:bugfix/web_app_ssl_resource_group

Conversation

@phekmat
Copy link
Copy Markdown
Contributor

@phekmat phekmat commented Apr 4, 2017

SSL certificates are associated with the the app service plan/server farm in Azure, so use the web app's server_farm_id to extract the associated resource group for any bind, unbind, or upload actions. Fixes #2750

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

General Guidelines

  • The PR has modified HISTORY.rst with an appropriate description of the change.

Command Guidelines

  • Each command and parameter has a meaningful description.
  • Each new command has a test.

(see Authoring Command Modules)

@msftclas
Copy link
Copy Markdown

msftclas commented Apr 4, 2017

@phekmat,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@phekmat
Copy link
Copy Markdown
Contributor Author

phekmat commented Apr 4, 2017

Feel free to run with this if you need to. I grabbed _get_resource_group_name_by_resource_id from

azure-cli/src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_utils.py

Line 40 in 4c98204

def get_resource_group_name_by_resource_id(resource_id):
. It might be worth extracting it elsewhere.

@phekmat phekmat force-pushed the bugfix/web_app_ssl_resource_group branch from 1c8d3cf to be59ad8 Compare April 4, 2017 23:40
@msftclas
Copy link
Copy Markdown

msftclas commented Apr 4, 2017

@phekmat, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@phekmat phekmat force-pushed the bugfix/web_app_ssl_resource_group branch from be59ad8 to 51d31b7 Compare April 5, 2017 02:29
@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 5, 2017 •
edited
Loading

Codecov Report

Merging #2755 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2755      +/-   ##
==========================================
+ Coverage   62.86%   62.86%   +<.01%     
==========================================
  Files         480      480              
  Lines       25886    25888       +2     
  Branches     3923     3923              
==========================================
+ Hits        16273    16275       +2     
  Misses       8599     8599              
  Partials     1014     1014
Impacted Files Coverage Δ
...ice/azure/cli/command_modules/appservice/custom.py 74.44% <100%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd467e7...0c46b24. Read the comment docs.

@phekmat
Copy link
Copy Markdown
Contributor Author

phekmat commented Apr 5, 2017

I tried to create a VCR-based test case for this as well, but it looks like the appservice web create command has a similar problem. It expects the service plan to be in the same resource group as the web app.

@derekbekoe derekbekoe added the Web Apps az webapp label Apr 5, 2017
yugangw-msft
yugangw-msft reviewed Apr 6, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@yugangw-msft yugangw-msft left a comment

Choose a reason for hiding this comment

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

@LukaszStem, please take a look and confirm the logic

src/command_modules/azure-cli-appservice/azure/cli/command_modules/appservice/custom.py Outdated
Copy link
Copy Markdown
Contributor

@yugangw-msft yugangw-msft Apr 6, 2017

Choose a reason for hiding this comment

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

please replace with from azure.cli.core.commands.arm import parse_resource_id and use that common routine

Copy link
Copy Markdown
Contributor Author

@phekmat phekmat Apr 6, 2017

Choose a reason for hiding this comment

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

Done (it was already imported, so I just had to change the usages). Thanks

Use the app service plan's resource group for cert operations
0c46b24 
SSL certificates are associated with the the app service plan/server farm in
Azure, so use the web app's `server_farm_id` to extract the associated resource
group for any `bind`, `unbind`, or `upload` actions. Fixes Azure#2750
@phekmat phekmat force-pushed the bugfix/web_app_ssl_resource_group branch from 51d31b7 to 0c46b24 Compare April 6, 2017 17:49
@yugangw-msft yugangw-msft merged commit 4c7e7b7 into Azure:master Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@yugangw-msft yugangw-msft yugangw-msft approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@phekmat @msftclas @codecov-io @yugangw-msft @haroldrandom @derekbekoe