-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Compute] Add AppGateway support to VMSS create #2570
Conversation
Updated help text:
|
Codecov Report
@@ Coverage Diff @@
## master #2570 +/- ##
==========================================
- Coverage 71.95% 71.79% -0.17%
==========================================
Files 375 375
Lines 20602 20689 +87
Branches 3012 3035 +23
==========================================
+ Hits 14825 14854 +29
- Misses 4846 4901 +55
- Partials 931 934 +3
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.
I suggest we name every gateway specific argument to start with "--application-gateway", and same on "--load-balancer". This way I can easily ignore not related arguments per balancer type. If we prefer shorter name, we can go with '--app-gateway' and '--lb'?
The rest LGTM.
@@ -0,0 +1,205 @@ | |||
{ | |||
"$schema": "http://schema.management.azure.com/schemas/2015-01-01-preview/deploymentTemplate.json", |
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 we need to check in this file?
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.
No, I was just using it as reference. Good catch.
@@ -481,7 +508,7 @@ def _validate_vm_create_public_ip(namespace): | |||
|
|||
|
|||
def _validate_vmss_create_public_ip(namespace): | |||
if namespace.load_balancer_type is None: | |||
if namespace.load_balancer_type is None and namespace.app_gateway_type is None: | |||
if namespace.public_ip_address: | |||
raise CLIError('--public-ip-address is not applicable when there is no load-balancer ' | |||
'attached, or implictly disabled due to 100+ instance count') |
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.
We should update the error message to incorporate app-gateway
if not namespace.single_placement_group: | ||
if not namespace.single_placement_group and namespace.load_balancer: | ||
raise CLIError( | ||
'--load-balancer is not applicable when --single-placement-group is turned off.') |
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 think we need to double check this error message. My understanding is the scenario is still supported that you can have <=100
machine and --single-placement-group
is turned off
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 original error was:
'--load-balancer is not applicable when --single-placement-group is explictly turned off or implictly turned off for 100+ instance count'
This would be the "explicitly turned off" case.
azure-cli.pyproj
Outdated
@@ -15,8 +15,7 @@ | |||
<InterpreterId>{54f4b6dc-0859-46dc-99bb-b275c9d0aca3}</InterpreterId> | |||
<InterpreterVersion>3.5</InterpreterVersion> | |||
<EnableNativeCodeDebugging>False</EnableNativeCodeDebugging> | |||
<CommandLineArguments> | |||
</CommandLineArguments> | |||
<CommandLineArguments>vmss create -g tjp-vmss -n vmss1 --image UbuntuLTS --authentication-type password --admin-password TestTest12#$ --validate --verbose</CommandLineArguments> |
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.
Remove this?
@yugangw-msft regarding the parameter names, if I change the existing ones it would be a breaking change. I could introduce aliases for those. So: |
Here's the updated help text:
|
Closes #2335. Permits creation of an app gateway as the frontend for VMSS.
Known issues:
Supported Scenarios
Unsupported Scenarios