Websites: Add Multicontainer Capabilities by LukaszStem · Pull Request #6255 · Azure/azure-cli

LukaszStem · 2018-04-28T18:15:24Z

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.

promptws · 2018-04-28T18:15:47Z

View a preview at https://prompt.ws/r/Azure/azure-cli/6255
This is an experimental preview for @microsoft users.

yugangw-msft

Good start! Please address my comments

yugangw-msft · 2018-04-28T20:15:27Z

        c.argument('docker_registry_server_user', options_list=['--docker-registry-server-user', '-u'], help='the container registry server username')
        c.argument('docker_registry_server_password', options_list=['--docker-registry-server-password', '-p'], help='the container registry server password')
        c.argument('websites_enable_app_service_storage', options_list=['--enable-app-service-storage', '-t'], help='enables platform storage (custom container only)', arg_type=get_three_state_flag(return_label=True))
+        c.argument('multicontainer_config_type', options_list=['--multicontainer-config-type'], help='config type', arg_type=get_enum_type(MULTI_CONTAINER_TYPES))


is this linux only as well like the one used in webapp create? I suggested we could consolidate them to avoid dupes.

yugangw-msft · 2018-04-28T20:16:36Z

+        if not validate_linux_create_options(runtime, deployment_container_image_name,
+                                             multicontainer_config_type, multicontainer_config_file):
+            raise CLIError("usage error: --runtime | --deployment-container-image-name |"
+                           " --multicontainer-config-type with --multicontainer-config-file")


Per convention, you can just say --multicontainer-config-type TYPE --multicontainer-config-file FILE. with is unnecessary

yugangw-msft · 2018-04-28T20:20:54Z

    elif runtime:  # windows webapp with runtime specified
        if startup_file or deployment_container_image_name:
-            raise CLIError("usage error: --startup-file or --deployment-container-image-name is "
+            raise CLIError("usage error: --startup-file or --deployment-container-image-name or "


you will need to add new sub-conditions with multicontainer_config_type or multicontainer_config_file

yugangw-msft · 2018-04-28T20:23:11Z

+                                  multicontainer_config_type=None, multicontainer_config_file=None):
+    if bool(multicontainer_config_type) != bool(multicontainer_config_file):
+        return False
+    opts = [bool(runtime), bool(deployment_container_image_name), bool(multicontainer_config_type)]


Minor: you can remove the bool() to leverage Python's support for True Value testing

yugangw-msft · 2018-04-28T20:25:37Z



-def _format_linux_fx_version(custom_image_name):
+def _format_linux_fx_version(custom_image_name, custom_prefix=None):


Unless custom is a agreed-on term, shall we rename to "container_config_type" to make it more explicit?

yugangw-msft · 2018-04-28T20:32:47Z

+    if file_name:
+        if url_validator(file_name):
+            from urllib.request import urlopen
+            response = urlopen(file_name)


this will not work across platform, particularly windows and linux, as you need to set the SSL context. Please check out how acs does
Or, for this version, just pull out the support for opening from url

yugangw-msft · 2018-04-28T20:40:27Z

+            response = urlopen(file_name)
+            config_file_bytes = response.read()
+        else:
+            with open(file_name) as f:


since you will encode back in 2 lines below, you are better off just reading the file in binary mode, so why not with open(file_name, 'rb')?

yugangw-msft · 2018-04-28T20:50:33Z

+        try:
+            config_file_bytes = b64decode(linux_fx_version.split('|')[1].encode('utf-8'))
+        except:
+            raise CLIError('Could not decode config')


I suggest get rid of this if block all together, and just keep an string.
At high level, I suggest make the function do one thing which is to read file content and return a string. No other things. This way the function will have a bit of readability. In details:

please get rid of loops from encoding->decode->encoding again.

get rid of the encoded arguments, and let caller (only once place) to encode if needs

yugangw-msft · 2018-04-28T20:53:31Z

        update_app_settings(cmd, resource_group_name, name, settings, slot)
    settings = get_app_settings(cmd, resource_group_name, name, slot)

+    if bool(multicontainer_config_file) != bool(multicontainer_config_type):


I thought the multicontainer_config_file should be used together with multicontainer_config_type, why the opposite?

…ntainer

yugangw-msft

please address my new comments, and also test it well

yugangw-msft · 2018-04-30T16:32:16Z

        elif deployment_container_image_name:
            site_config.linux_fx_version = _format_linux_fx_version(deployment_container_image_name)
            site_config.app_settings.append(NameValuePair("WEBSITES_ENABLE_APP_SERVICE_STORAGE", "false"))
-        else:  # must specify runtime


why you remove this? Now we won't report anything if none of the arguments are supplied

validate_linux_create_options() will catch this right?

yugangw-msft · 2018-04-30T16:34:28Z

    elif runtime:  # windows webapp with runtime specified
-        if startup_file or deployment_container_image_name:
-            raise CLIError("usage error: --startup-file or --deployment-container-image-name is "
+        if startup_file or deployment_container_image_name or multicontainer_config_file or multicontainer_config_type:


why not use use any[...], to be consistent with the rest code you wrote?

yugangw-msft · 2018-04-30T16:39:53Z

+    try:
+        result = urlparse(url)
+        return all([result.scheme, result.netloc, result.path])
+    except:


this would fail the pylint. I believe you should only catch ValueError per this API's document

yugangw-msft · 2018-04-30T16:42:28Z

+        raise CLIError("Cannot decode config that is not one of the"
+                       " following types: {}".form    (','(MULTI_CONTAINER_TYPES)))
+    try:
+        return b64decode(linux_fx_version.spl('|'[1].encode('utf-8')))


I don't think this code would work at all. have you tested it?

yugangw-msft · 2018-04-30T16:44:16Z

+         response = urlopen(file_name, context=_ssl_context())
+         config_file_bytes = response.read()
+     else:
+         with open(file_name) as f:


you can read the file as binary, so you don't need to encode again in the 2 line below

yugangw-msft · 2018-04-30T16:45:04Z

        update_app_settings(cmd, resource_group_name, name, settings, slot)
    settings = get_app_settings(cmd, resource_group_name, name, slot)

+    if bool(multicontainer_config_file) and bool(multicontainer_config_type):


you can simply: multicontainer_config_file and multicontainer_config_type

yugangw-msft · 2018-04-30T16:47:41Z

+        linux_fx_version = _format_linux_fx_version(encoded_config_file, multicontainer_config_type)
+        update_site_configs(cmd, resource_group_name, name, linux_fx_version=linux_fx_version)
+    else:
+        raise CLIError('Must use --multicontainer-config-file FILE --multicontainer-config-type TYPE')


This could be very wrong, unless I mis-understood. To me now this command would only succeed with multicontainer_config_file and multicontainer_config_type; while all other settings would not go through

yugangw-msft

Looks much better now.
One more comment, and keep on testing it

yugangw-msft · 2018-04-30T18:24:01Z

should not this be multicontainer_config_file or multicontainer_config_type? Otherwise, setting an unrelated config like docker_custom_image_name would trigger this warn.

yugangw-msft · 2018-04-30T18:26:35Z

Can you log an issue and assign to yourself, so you can improve the test to create all from the scratch instead of hard-coding the assert?

yugangw-msft · 2018-04-30T19:22:31Z

please use from six.moves.urllib.request import urlopen like other command module

yugangw-msft · 2018-04-30T19:22:42Z

        elif deployment_container_image_name:
            site_config.linux_fx_version = _format_linux_fx_version(deployment_container_image_name)
            site_config.app_settings.append(NameValuePair("WEBSITES_ENABLE_APP_SERVICE_STORAGE", "false"))
-        else:  # must specify runtime


yugangw-msft · 2018-04-30T19:24:51Z

i suggest you don't catch, let it throw unless you can make the error specific

yugangw-msft · 2018-04-30T19:25:47Z

like i said, you will have to fix the else here to avoid impacting common case

yugangw-msft

One more comment to revert an relevant change. Also do enough testing. After these, let me know when I should merge

yugangw-msft · 2018-04-30T19:58:58Z

please rebase to get rid of the diff on this file

…ntainer

LukaszStem · 2018-04-30T22:38:34Z

LGTM

LukaszStem requested a review from yugangw-msft April 28, 2018 18:15

LukaszStem force-pushed the multicontainer branch from 129d739 to d238802 Compare April 28, 2018 18:30

LukaszStem changed the title ~~Websites Multicontainer change~~ Websites: Add Multicontainer Capabilities Apr 28, 2018

Websites Multicontainer change

68e0227

LukaszStem force-pushed the multicontainer branch from c5da61f to 68e0227 Compare April 28, 2018 18:41

yugangw-msft reviewed Apr 28, 2018

View reviewed changes

Lukasz Stempniewicz added 2 commits April 29, 2018 16:49

Merge branch 'dev' of https://github.com/Azure/azure-cli into multico…

9d383bf

…ntainer

Addressed comments

dbb2757

yugangw-msft suggested changes Apr 30, 2018

View reviewed changes

derekbekoe added this to the Sprint 36 (Build) milestone Apr 30, 2018

derekbekoe assigned yugangw-msft Apr 30, 2018

Fixed errors and retested

6787ac7

yugangw-msft reviewed Apr 30, 2018

View reviewed changes

LukaszStem force-pushed the multicontainer branch from bc8cf61 to e0b0e99 Compare April 30, 2018 18:31

yugangw-msft reviewed Apr 30, 2018

View reviewed changes

LukaszStem force-pushed the multicontainer branch 2 times, most recently from a16701c to 87bd7fc Compare April 30, 2018 19:33

LukaszStem requested review from derekbekoe and troydai April 30, 2018 19:33

LukaszStem mentioned this pull request Apr 30, 2018

Improve test_linux_webapp_multicontainer_create() #6263

Closed

yugangw-msft reviewed Apr 30, 2018

View reviewed changes

Comment thread scripts/ci/test_integration.sh Outdated

Copy link
Copy Markdown

Contributor

yugangw-msft Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rebase to get rid of the diff on this file

Lukasz Stempniewicz added 2 commits April 30, 2018 13:24

Fix pylint errors

c3787a9

Merge branch 'dev' of https://github.com/Azure/azure-cli into multico…

b192243

…ntainer

LukaszStem force-pushed the multicontainer branch from 1519908 to b192243 Compare April 30, 2018 20:26

Lukasz Stempniewicz added 2 commits April 30, 2018 14:24

Fix CI

fb9eb34

Rerecord various tests

b3bd113

LukaszStem force-pushed the multicontainer branch from 4fb1872 to b3bd113 Compare April 30, 2018 22:06

yugangw-msft approved these changes Apr 30, 2018

View reviewed changes

yugangw-msft merged commit c7641a4 into Azure:dev Apr 30, 2018



		def _format_linux_fx_version(custom_image_name):
		def _format_linux_fx_version(custom_image_name, custom_prefix=None):

Conversation

LukaszStem commented Apr 28, 2018

Uh oh!

promptws commented Apr 28, 2018

Uh oh!

yugangw-msft left a comment

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukaszStem commented Apr 30, 2018

Uh oh!

Reviewers

Assignees

Labels

yugangw-msft Apr 28, 2018 •

edited

Loading

yugangw-msft Apr 28, 2018 •

edited

Loading

yugangw-msft Apr 30, 2018 •

edited

Loading

yugangw-msft Apr 30, 2018 •

edited

Loading