-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 to create plan when create a webapp #2550
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2550 +/- ##
==========================================
+ Coverage 72.13% 72.29% +0.15%
==========================================
Files 362 363 +1
Lines 19776 19832 +56
Branches 2920 2923 +3
==========================================
+ Hits 14266 14338 +72
+ Misses 4589 4584 -5
+ Partials 921 910 -11
Continue to review full report at Codecov.
|
@yugangw-msft You are my hero! This looks awesome. |
sku_arg_type = CliArgumentType(help='The pricing tiers, e.g., F1(Free), D1(Shared), B1(Basic Small), B2(Basic Medium), B3(Basic Large), S1(Standard Small), P1(Premium Small), etc', | ||
**enum_choice_list(['F1', 'FREE', 'D1', 'SHARED', 'B1', 'B2', 'B3', 'S1', 'S2', 'S3', 'P1', 'P2', 'P3'])) | ||
_SKU_HELP = 'The pricing tiers, e.g., F1(Free), D1(Shared), B1(Basic Small), B2(Basic Medium), B3(Basic Large), S1(Standard Small), P1(Premium Small), etc' | ||
_SKU_LIST = ['F1', 'FREE', 'D1', 'SHARED', 'B1', 'B2', 'B3', 'S1', 'S2', 'S3', 'P1', 'P2', 'P3'] |
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.
Can we get this metadata from SDK? It doesn't feel right for SDK to hard-code service options.
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.
Agreed, it is not right. But it is a known webapp api pattern, that offers no enum or choices. Yep, this makes lives a bit harder for client apps
'plan {}. Please use --plan to specify a new plan'.format(namespace.plan)) | ||
|
||
|
||
|
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.
Extra empty line. Didn't pylint complain?
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.
Apparently not or it would not have passed CI 😁
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.
This will not be a pylint error, rather PEP 8 style check should catch it. I have opened #2562 to get that done.
from azure.cli.core._util import CLIError | ||
from azure.cli.command_modules.appservice._validators import process_webapp_create_namespace | ||
|
||
class TestValidators(unittest.TestCase): |
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.
Two empty lines between top level structures.
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.
This style check error will be fixed through #2562.
JMESPathCheck('length(@)', 0) | ||
]) | ||
|
||
class WebappSimpleCreateTest(ResourceGroupVCRTestBase): |
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.
I suggest write new tests using ScenarioTest base class.
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.
Done
from azure.cli.core._util import CLIError | ||
from azure.cli.core.commands.validators import get_default_location_from_resource_group | ||
|
||
def _validate_plan_arg(namespace): |
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.
I don't think it is necessary to initial the function name with an underline. The module name itself already indicate its a private module.
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.
Done
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.
Want to review this before merged.
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, looks good. A couple questions and I would suggest you implement Troy's recommendations before merging.
register_cli_argument('appservice web create', 'create_plan', ignore_type) | ||
register_cli_argument('appservice web create', 'plan', arg_group='AppService Plan', | ||
options_list=('--plan', '-p'), completer=get_resource_name_completion_list('Microsoft.Web/serverFarms'), | ||
help='Appservice plan name. Can also reference an existing subnet by ID. If omitted, an appropriate plan in the same resource group will be selected automatically, or a new one will be created.') |
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.
Copy/paste error? "Can also reference an existing subnet by ID"?
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.
Done
'plan {}. Please use --plan to specify a new plan'.format(namespace.plan)) | ||
|
||
|
||
|
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.
Apparently not or it would not have passed CI 😁
def _match_plan_location(plan, location): | ||
# the problem with appservice is it uses display name, rather canonical name | ||
# so we have to hack it | ||
return plan.location.replace(' ', '').lower() == location.lower() |
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.
Is this because of a bug?
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.
I would say, not a bug, but another anti-pattern in webapp's service.
webapp_def = Site(server_farm_id=plan, location=location) | ||
plan_id = plan | ||
if create_plan: | ||
logger.warning("Create appservice plan: '%s'", plan) |
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 issuing a warning? We don't do this for other creates (VM/VMSS/LB/AG) that create extra resources.
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.
I thought about it before put in a warning. The situation here is a bit different, that we are creating a parent
like resource which contains the webapp
, and sooner or later users will need to know the existence of the plan for scenarios like scale up/down. This is different with vnet
, subnet, etc
, which would be mostly transparent to users.
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
@lostintangent, I am merging this one. Please note, I decided to make a separate PR for the command renaming we chatted in #2325. The Webapp team is circulating the naming change and will confirm it no later than this Wednesday. |
* Azure/master: (478 commits) vm live test: allow more valid power states on vmss test verifications (Azure#2564) rbac:catch more graph error (Azure#2567) appservice: support to create plan when create a webapp (Azure#2550) Update storage tests (Azure#2556) Change PEP8 check filter from whitelist to blacklist (Azure#2557) Add scenario tests documentation (Azure#2555) [ACS] Adding support for configuring a default ACS cluster (Azure#2554) [ACS] Provide a short name alias for the orchestrator type flag (Azure#2553) Sql Import/Export CLI commands and test (Azure#2538) Fix format bug. (Azure#2549) [VM/VMSS] Improved disk caching support (Azure#2522) VM/VMSS: incorporate credentials validation logic used by portal (Azure#2537) Script that creates packaged releases package archive (Azure#2508) Adding alias for defaults flag (Azure#2540) Add wait commands and --no-wait support (Azure#2524) choice list outside of named arguments (Azure#2521) Fixed test failure in test_sql_db_mgmt. (Azure#2530) core: support login using service principal with a cert (Azure#2457) Add note about being in preview (Azure#2512) vm:fix distro check mechanism used by disk encryption (Azure#2511) ...
Fix #1369