Add acs module support for version 2017-07-01 #4010

rjtsdl · 2017-07-12T00:35:43Z

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 (see Modifying change log).

Command Guidelines

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

(see Authoring Command Modules)

TODO list

Test Recording files to be generated
Add help message for using multiple agentpool

msftclas · 2017-07-12T00:35:46Z

@rjtsdl,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

rjtsdl · 2017-07-12T23:38:41Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/tests/test_acs_commands.py

    @RoleBasedServicePrincipalPreparer()
    def test_acs_create_kubernetes(self, resource_group, sp_name, sp_password):
-        acs_name = self.create_random_name('cliacstest', 16)
+        acs_name = self.create_random_name('acs', 14)
        ssh_pubkey_file = self.generate_ssh_keys().replace('\\', '\\\\')
        cmd = 'acs create -g {} -n {} --orchestrator-type Kubernetes --service-principal {} ' \
              '--client-secret {} --ssh-key-value {}'


I was trying to add more tests.
After a second thought, the tests i want to add are more about the version 2017-07-01. It is not available everywhere now. So the test would be a bit weird.

I will defer the tests work for now.

rjtsdl · 2017-07-12T23:39:50Z

@brendandburns @anhowe, PTAL

rjtsdl · 2017-07-12T23:52:45Z

src/command_modules/azure-cli-acs/HISTORY.rst

+* api version 2017-07-01 support
+* update dcos and swarm to use latest api version instead of 2016-03-30
+* major refactoring
+


Highlight the changes.

'Major refactoring' isn't useful for the end-user.
If you can expand on that on any changes that affect your commands (what an end-user will see), please include that; otherwise I'd remove this line.

Make sense. I will remove the line.

derekbekoe · 2017-07-13T00:00:59Z

src/command_modules/azure-cli-acs/HISTORY.rst

+* api version 2017-07-01 support
+* update dcos and swarm to use latest api version instead of 2016-03-30
+* major refactoring
+


'Major refactoring' isn't useful for the end-user.
If you can expand on that on any changes that affect your commands (what an end-user will see), please include that; otherwise I'd remove this line.

derekbekoe · 2017-07-13T00:04:27Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

+        - name: Create a Kubernetes cluster with ssh key provided
+          text: az acs create --orchestrator-type Kubernetes -g <resource_group_name> -n <acs_cluster_name> --ssh-key-value <ssh_key_value_or_path>
+        - name: Create a acs cluster with two agent pools
+          text: az acs create -g <resource_group_name> -n <acs_cluster_name> --agent-profiles "[{'name':'agentpool1'},{'name':'agentpool2'}]"


<resource_group_name> -> MyResourceGroup
e.g. https://docs.microsoft.com/en-us/cli/azure/acs#create

Same with the other params here.

I think docs prefer that format as it can then be used directly whereas the angle brackets probably wouldn't be accepted by the service.

they are optional, and can be used to override specific pool's value

rjtsdl · 2017-07-13T01:47:29Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/acs_client.py

-        :param background: True if command should be run in the foreground,
-        false to run it in a separate thread
+        :param background: True to run it in a separate thread,
+               False should be run in the foreground
        :type command: Boolean


The comment doesn't make sense to me. So i correct it. PTAL

brendandburns · 2017-07-13T02:57:15Z

LGTM. I defer to @derekbekoe for final approval.

rjtsdl · 2017-07-13T03:54:16Z

@derekbekoe, here is the error of pr test failure. Only for python 2.7 one. The other two passed.
The error seems not from this module. It is from azure-cli-core. Do you know how can i make the pr test pass?

Run pylint
Modules: azure-cli-core, azure-cli-testsdk, azure-cli, acr, acs, appservice, batch, billing, cdn, cloud, cognitiveservices, component, configure, consumption, cosmosdb, dla, dls, feedback, find, interactive, iot, keyvault, lab, monitor, network, profile, rdbms, redis, resource, role, sf, sql, storage, taskhelp, vm
Telemetry upload begins
Traceback (most recent call last):
  File "/home/travis/build/Azure/azure-cli/src/azure-cli-core/azure/cli/core/telemetry_upload.py", line 68, in <module>
    upload(sys.argv[1])
  File "/home/travis/build/Azure/azure-cli/src/azure-cli-core/azure/cli/core/decorators.py", line 86, in _wrapped_func
    raise ex
TypeError: can't multiply sequence by non-int of type 'str'
************* Module azure.cli.command_modules.acs.custom
E:633, 0: invalid syntax (<string>, line 633) (syntax-error)

rjtsdl · 2017-07-13T16:50:11Z

I fix a place where i use some python 3.0+ supported syntax. It probably related to the error above. I am still confused by the error message though.

codecov-io · 2017-07-13T17:10:35Z

Codecov Report

Merging #4010 into master will decrease coverage by 0.01%.
The diff coverage is 70.31%.

@@            Coverage Diff            @@
##           master   #4010      +/-   ##
=========================================
- Coverage   70.51%   70.5%   -0.02%     
=========================================
  Files         428     428              
  Lines       27559   27609      +50     
  Branches     4218    4229      +11     
=========================================
+ Hits        19433   19465      +32     
- Misses       6873    6883      +10     
- Partials     1253    1261       +8

Impacted Files	Coverage Δ
...li-acs/azure/cli/command_modules/acs/acs_client.py	`46.72% <ø> (ø)`	⬆️
...ure-cli-acs/azure/cli/command_modules/acs/_help.py	`100% <100%> (ø)`	⬆️
...e-cli-acs/azure/cli/command_modules/acs/_params.py	`75.71% <100%> (+6.07%)`	⬆️
...i-acs/azure/cli/command_modules/acs/_validators.py	`40% <50%> (+0.71%)`	⬆️
...re-cli-acs/azure/cli/command_modules/acs/custom.py	`43.24% <60.86%> (+0.33%)`	⬆️

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 9eb33c1...9fac6cd. Read the comment docs.

rjtsdl · 2017-07-13T17:34:21Z

@derekbekoe , how can i get write access? Not able to merge :(
Or you can help me merge :)

msftclas added the cla-not-required label Jul 12, 2017

rjtsdl changed the title ~~Add acs module support for version 2017-07-01~~ [WIP] Add acs module support for version 2017-07-01 Jul 12, 2017

rjtsdl changed the title ~~[WIP] Add acs module support for version 2017-07-01~~ [WIP]: Add acs module support for version 2017-07-01 Jul 12, 2017

rjtsdl force-pushed the jiren-dev branch 4 times, most recently from 0770598 to 920a8b1 Compare July 12, 2017 23:33

rjtsdl commented Jul 12, 2017

View reviewed changes

rjtsdl changed the title ~~[WIP]: Add acs module support for version 2017-07-01~~ Add acs module support for version 2017-07-01 Jul 12, 2017

rjtsdl commented Jul 12, 2017

View reviewed changes

derekbekoe requested a review from brendandburns July 12, 2017 23:59

derekbekoe suggested changes Jul 13, 2017

View reviewed changes

Jingtao Ren added 16 commits July 12, 2017 18:36

ignore .python-version file

630e7be

correct comments for acs_client.py run()

7e51668

add api-version option for az acs create

f057a3c

Introduce --master-profile and --agent-profiles options for az acs

f16c674

they are optional, and can be used to override specific pool's value

add master-vm-size parameter

92be3c8

add osDiskSizeGB options for both master and agent

bd3a53b

add vnetSubnetID for both master and agent pools

c8dfd3b

whitelist 2017-07-01 regions

2dd393c

add firstConsecutiveStaticIP option to master profile

8577729

add agent_ports option

e7b2b77

add storageProfile options for master and agent

42c86d5

consolidate create for k8s and non-k8s

2801ab7

debugging module acs

434498a

change HISTORY.rst

c5347e8

fix lint

3d685ca

correct latest/recording test yaml files, and add helps examples

bbd44d4

rjtsdl force-pushed the jiren-dev branch from 920a8b1 to f0d2269 Compare July 13, 2017 01:36

address comments

c7676c6

rjtsdl force-pushed the jiren-dev branch from f0d2269 to c7676c6 Compare July 13, 2017 01:38

fix ManagedDisks spelling

94d9934

rjtsdl commented Jul 13, 2017

View reviewed changes

use update instead of **{} syntax, which is only for python 3.0+

9fac6cd

rjtsdl force-pushed the jiren-dev branch from adcedb0 to 9fac6cd Compare July 13, 2017 17:00

derekbekoe approved these changes Jul 13, 2017

View reviewed changes

derekbekoe merged commit 2a59cee into Azure:master Jul 13, 2017

rjtsdl deleted the jiren-dev branch July 13, 2017 17:48

haroldrandom added the cla-not-required label Oct 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add acs module support for version 2017-07-01 #4010

Add acs module support for version 2017-07-01 #4010

rjtsdl commented Jul 12, 2017 •

edited

Loading

msftclas commented Jul 12, 2017

rjtsdl Jul 12, 2017

rjtsdl commented Jul 12, 2017

rjtsdl Jul 12, 2017

derekbekoe Jul 13, 2017

rjtsdl Jul 13, 2017

derekbekoe Jul 13, 2017

derekbekoe Jul 13, 2017

rjtsdl Jul 13, 2017

brendandburns commented Jul 13, 2017

rjtsdl commented Jul 13, 2017

rjtsdl commented Jul 13, 2017

codecov-io commented Jul 13, 2017 •

edited

Loading

rjtsdl commented Jul 13, 2017 •

edited

Loading

Add acs module support for version 2017-07-01 #4010

Add acs module support for version 2017-07-01 #4010

Conversation

rjtsdl commented Jul 12, 2017 • edited Loading

General Guidelines

Command Guidelines

TODO list

msftclas commented Jul 12, 2017

rjtsdl Jul 12, 2017

Choose a reason for hiding this comment

rjtsdl commented Jul 12, 2017

rjtsdl Jul 12, 2017

Choose a reason for hiding this comment

derekbekoe Jul 13, 2017

Choose a reason for hiding this comment

rjtsdl Jul 13, 2017

Choose a reason for hiding this comment

derekbekoe Jul 13, 2017

Choose a reason for hiding this comment

derekbekoe Jul 13, 2017

Choose a reason for hiding this comment

rjtsdl Jul 13, 2017

Choose a reason for hiding this comment

brendandburns commented Jul 13, 2017

rjtsdl commented Jul 13, 2017

rjtsdl commented Jul 13, 2017

codecov-io commented Jul 13, 2017 • edited Loading

Codecov Report

rjtsdl commented Jul 13, 2017 • edited Loading

rjtsdl commented Jul 12, 2017 •

edited

Loading

codecov-io commented Jul 13, 2017 •

edited

Loading

rjtsdl commented Jul 13, 2017 •

edited

Loading