Skip to content
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: make SKU optional when creating an appservice plan #1383

Merged
merged 2 commits into from
Nov 18, 2016

Conversation

mayurid
Copy link
Member

@mayurid mayurid commented Nov 18, 2016

Closes #1381

@mention-bot
Copy link

@mayurid, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yugangw-msft, @tjprescott and @derekbekoe to be potential reviewers.

@derekbekoe
Copy link
Member

I think we should use the reflection we do on the methods for this.
There was a bug there in the method definition.
In the method linked below, we should have sku='B1' instead of sku.

And then, we can remove default='B1' from the params file.

https://github.com/Azure/azure-cli/blob/master/src/command_modules/azure-cli-appservice/azure/cli/command_modules/appservice/custom.py#L252

@yugangw-msft
Copy link
Contributor

I author'd the defaults in the _param.py so all related literals will be in the same file for easier update in future. I am fine to leverage reflections, since if there are mismatch, the test will catch it anyway.

@mayurid
Copy link
Member Author

mayurid commented Nov 18, 2016

@derekbekoe : Addressed the feedback. @yugangw-msft : Thanks for helping with it

@derekbekoe
Copy link
Member

@mayurid I added 'Closes #1381' to the PR description so the issue closes when the PR is merged.

link: https://github.com/blog/1506-closing-issues-via-pull-requests

Copy link
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

LGTM

@tjprescott tjprescott merged commit bbc0316 into Azure:master Nov 18, 2016
@mayurid mayurid deleted the optional branch November 18, 2016 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants