Add support for windows clusters. #2211
Conversation
| @@ -399,6 +399,10 @@ def acs_create(resource_group_name, deployment_name, name, ssh_key_value, dns_na | |||
| :param tags: Tags object. | |||
| :type tags: object | |||
| :param dict custom_headers: headers that will be added to the request | |||
| :param windows: If true, the cluster will be built for running windows container. | |||
| if windows: | ||
| windows_profile = { | ||
| "adminUsername": admin_username, | ||
| "adminPassword": admin_password, |
There was a problem hiding this comment.
Is there validation in the CLI for Windows passwords and can it be used here? The cloud user account policy is here: https://docs.microsoft.com/en-us/azure/active-directory/active-directory-passwords-policy
There was a problem hiding this comment.
There's no validation in there yet (even for VMs) I'll try to add it in a different PR.
There was a problem hiding this comment.
will the current default value of empty string work? If not, i suggest put in a line to just throw
There was a problem hiding this comment.
FYI, VM's password validation is here
1e23cfd
to
785a04f
Compare
|
Thanks for the review. @yugang-msft can I can a review for this? |
| @@ -350,7 +350,7 @@ def acs_create(resource_group_name, deployment_name, name, ssh_key_value, dns_na | |||
| content_version=None, admin_username="azureuser", agent_count="3", | |||
| agent_vm_size="Standard_D2_v2", location=None, master_count="3", | |||
| orchestrator_type="dcos", service_principal=None, client_secret=None, tags=None, | |||
| custom_headers=None, raw=False, | |||
| custom_headers=None, windows=False, admin_password="", raw=False, | |||
There was a problem hiding this comment.
Suggest update the params.py file to make windows as a flag, rather with values true and false
| if windows: | ||
| windows_profile = { | ||
| "adminUsername": admin_username, | ||
| "adminPassword": admin_password, |
There was a problem hiding this comment.
will the current default value of empty string work? If not, i suggest put in a line to just throw
| if windows: | ||
| windows_profile = { | ||
| "adminUsername": admin_username, | ||
| "adminPassword": admin_password, |
There was a problem hiding this comment.
FYI, VM's password validation is here
785a04f
to
824725f
Compare
|
Comments addressed. ptal. Thanks |
| @@ -70,6 +70,8 @@ def _get_default_install_location(exe_name): | |||
| register_extra_cli_argument('acs create', 'generate_ssh_keys', action='store_true', help='Generate SSH public and private key files if missing') | |||
| register_cli_argument('acs create', 'agent_vm_size', completer=get_vm_size_completion_list) | |||
|
|
|||
| register_cli_argument('acs create', 'windows', action='store_false', help='If true, deploy a windows container cluster.') | |||
There was a problem hiding this comment.
why not store_true? This automatically creates a default value of False, which I believe is you want
store_false will default to True when the command-line argument is not present. Is that what you want?
There was a problem hiding this comment.
That makes no sense to me :P, but you are right I want a default value of False...
There was a problem hiding this comment.
Agreed the naming is a bit odd. It is from the built-in argparse package which CLI builds on top of
| os_type = 'Linux' | ||
| if windows: | ||
| if len(admin_password) == 0: | ||
| raise CLIError('--admin-password is required.') |
There was a problem hiding this comment.
The upcoming check of <6 is good enough, so I suggest we just remove this if
There was a problem hiding this comment.
I'd prefer to have a cleaner error messages.
824725f
to
82ac4df
Compare
|
Changes addressed, please re-check. Thanks! |
|
@brendandburns could you resolve the conflict, so i can merge? |
|
@yugangw-msft, done. Thanks |
Codecov Report
@@ Coverage Diff @@
## master #2211 +/- ##
=========================================
- Coverage 72.24% 72.2% -0.05%
=========================================
Files 323 323
Lines 18086 18098 +12
Branches 2650 2654 +4
=========================================
+ Hits 13067 13068 +1
- Misses 4196 4206 +10
- Partials 823 824 +1
Continue to review full report at Codecov.
|
@anhowe