Add Kubernetes support to ACS. #1258

brendandburns · 2016-11-07T19:02:50Z

@yugang-msft

Thanks!
--brendan

yugangw-msft

Please fix pylint errors and get CI green. Also clarify review comments.

yugangw-msft · 2016-11-07T21:10:37Z

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


 cli_command('acs dcos browse', dcos_browse)
 cli_command('acs dcos install-cli', dcos_install_cli)
+cli_command('acs create', acs_create)
+cli_command('acs dcos browse', dcos_browse)


please delete this line, as this is a duplicate of 3 lines above

yugangw-msft · 2016-11-07T21:11:22Z

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

+    file_url = ''
+    system = platform.system()
+    if system == 'Windows':
+        file_url = 'https://storage.googleapis.com/kubernetes-release/release/v1.4.4/bin/windows/amd64/kubectl.exe'


Consider to exposing the version like you did for dcos. Up to you at this moment though

yugangw-msft · 2016-11-07T21:12:17Z

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

+        sys.stdout.write ('.')
+        sys.stdout.flush()
+        time.sleep(2 + 2 * x)
+    print('done')


use sys.stdout.write just be consistent?

I prefer print for it's platform indepdent newline.

derekbekoe · 2016-11-07T22:04:57Z

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


 import azure.cli.core._logging as _logging
 from azure.cli.command_modules.acs import acs_client, proxy
+from azure.cli.command_modules.vm.mgmt_acs.lib import \
+    AcsCreationClient as ACSClient


mgmt_acs should be moved into this command module rather than the acs module having a dependency on vm.

@derekbekoe, this will be cleaned up in 2 weeks per conversation with Brendan

ack. I promise to fix this asap, but I need to get this in for the 11/11 cut.

Thanks for the flexibility.

yugangw-msft

More pylint errors to clean up

yugangw-msft · 2016-11-08T00:01:46Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

@@ -168,7 +168,7 @@ def _deploy_arm_template_core(resource_group_name, template_file=None, template_
        parameters = json.loads(parameters)
        if parameters:
            parameters = parameters.get('parameters', parameters)
-
+    


this change is unnecessary and might well cause a pylint errir

yugangw-msft · 2016-11-08T00:05:41Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_params.py

@@ -79,6 +79,8 @@ def get_vm_size_completion_list(prefix, action, parsed_args, **kwargs):#pylint:
 register_cli_argument('acs', 'container_service_name', options_list=('--name', '-n'), help='The name of the container service', completer=get_resource_name_completion_list('Microsoft.ContainerService/ContainerServices'))
 register_cli_argument('acs create', 'agent_vm_size', completer=get_vm_size_completion_list)
 register_cli_argument('acs scale', 'new_agent_count', type=int, help='The number of agents for the cluster')
+register_cli_argument('acs create', 'service_principal', help='Service principal for making calls into Azure APIs')
+register_cli_argument('acs create', 'client_secret', help='Client secret to use with the service principal for making calls to Azure APIs')


For your consideration, azure-cli commands use password terminology across for this concept. For consistency I would suggest you use the password here. Both terminologies are correct, but have used at different layers.
At this point it is your call, because you have to update related method as well

yugangw-msft · 2016-11-08T05:31:53Z

@brendandburns , other than pylint error, you should take a look at the test failure of acs create particularly the command no longer return the 3 pieces of information to user such as masterFQDN, agentFQDN, and ssh command. So you probably should incorporate the output artifacts from the quick start template. Please chat with @sauryadas for more context

brendandburns · 2016-11-08T06:37:29Z

@yugangw-msft I actually see that output in the example output that the JMESPath assertion is testing. I'm really not certain why the query isn't picking it up.

Is there a quick way to just run that one test?

Thanks
--brendan

yugangw-msft · 2016-11-08T17:30:16Z

~~@brendandburns, can you paste the output of the create ?~~
I know the cause, please refer to my later comment below

yugangw-msft · 2016-11-08T20:05:48Z

@brendandburns, yes you can run that test:

python -m unittest azure.cli.command_modules.vm.tests.test_vm_commands.AzureContainerServiceScenarioTest.test_acs_create_update

and to fix the test (assume you have communicated with @sauryadas), is to use fully qualified property path such as properties.outputs.masterFQDN.value. The reason for the change is the command now outputs the full blown object, rather the old customized slimmed down version

yugangw-msft · 2016-11-09T19:29:06Z

@brendandburns,
For JMESPATH in test, please ensure to point to "properties.outputs.masterFQDN.value"
For pylints error on using print, you should put ‘from future import print_function’ at the top

derekbekoe · 2016-11-09T22:45:57Z

PR #1059 has been merged so a rebase will be required with a couple of changes to the acs/commands.py and vm/commands.py files.

Changes required in acs/commands.py:

Remove the from .custom (...) import statements.
Change command registration (see snippets)

cli_command('acs create', acs_create)
cli_command('acs kubernetes install-cli', k8s_install_cli)
cli_command('acs kubernetes get-credentials', acs_get_credentials)

Would change to:

cli_command(__name__, 'acs create', 'azure.cli.command_modules.acs.custom#acs_create')
cli_command(__name__, 'acs kubernetes install-cli', 'azure.cli.command_modules.acs.custom#k8s_install_cli')
cli_command(__name__, 'acs kubernetes get-credentials', 'azure.cli.command_modules.acs.custom#get')

Changes required for the vm/commands.py conflict:

Remove the registration of the acs create command from this file.
In vm/_client_factory.py, remove the method cf_acs_create()

brendandburns · 2016-11-09T22:55:32Z

@derekbekoe yep, I made that refactor, ptal.

thanks!
--brendan

azurecla added the cla-not-required label Nov 7, 2016

yugangw-msft reviewed Nov 7, 2016

View reviewed changes

derekbekoe reviewed Nov 7, 2016

View reviewed changes

yugangw-msft reviewed Nov 8, 2016

View reviewed changes

brendandburns force-pushed the download-k8s-2 branch from d9500f1 to 7cb69e5 Compare November 8, 2016 01:29

brendandburns force-pushed the download-k8s-2 branch from 7cb69e5 to c2b8ba2 Compare November 8, 2016 06:36

brendandburns force-pushed the download-k8s-2 branch 2 times, most recently from 1a41ffd to de94d0e Compare November 9, 2016 22:30

brendandburns force-pushed the download-k8s-2 branch from de94d0e to 1182d91 Compare November 9, 2016 22:55

brendandburns force-pushed the download-k8s-2 branch 5 times, most recently from aeb5496 to 1038577 Compare November 10, 2016 02:30

Add Kubernetes support to ACS.

fe3d4bc

brendandburns force-pushed the download-k8s-2 branch from 1038577 to fe3d4bc Compare November 10, 2016 04:12

yugangw-msft merged commit 570387c into Azure:master Nov 10, 2016

derekbekoe mentioned this pull request Dec 7, 2016

[ACS] Update the handling of service principals. #1469

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Kubernetes support to ACS. #1258

Add Kubernetes support to ACS. #1258

brendandburns commented Nov 7, 2016

yugangw-msft left a comment

yugangw-msft Nov 7, 2016

brendandburns Nov 7, 2016

yugangw-msft Nov 7, 2016 •

edited

Loading

brendandburns Nov 7, 2016

yugangw-msft Nov 7, 2016

brendandburns Nov 7, 2016

derekbekoe Nov 7, 2016

yugangw-msft Nov 7, 2016

brendandburns Nov 7, 2016

yugangw-msft left a comment

yugangw-msft Nov 8, 2016

brendandburns Nov 9, 2016

yugangw-msft Nov 8, 2016

yugangw-msft commented Nov 8, 2016 •

edited

Loading

brendandburns commented Nov 8, 2016

yugangw-msft commented Nov 8, 2016 •

edited

Loading

yugangw-msft commented Nov 8, 2016 •

edited

Loading

yugangw-msft commented Nov 9, 2016

derekbekoe commented Nov 9, 2016 •

edited

Loading

brendandburns commented Nov 9, 2016

Add Kubernetes support to ACS. #1258

Add Kubernetes support to ACS. #1258

Conversation

brendandburns commented Nov 7, 2016

yugangw-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft commented Nov 8, 2016 • edited Loading

brendandburns commented Nov 8, 2016

yugangw-msft commented Nov 8, 2016 • edited Loading

yugangw-msft commented Nov 8, 2016 • edited Loading

yugangw-msft commented Nov 9, 2016

derekbekoe commented Nov 9, 2016 • edited Loading

brendandburns commented Nov 9, 2016

yugangw-msft Nov 7, 2016 •

edited

Loading

yugangw-msft commented Nov 8, 2016 •

edited

Loading

yugangw-msft commented Nov 8, 2016 •

edited

Loading

yugangw-msft commented Nov 8, 2016 •

edited

Loading

derekbekoe commented Nov 9, 2016 •

edited

Loading