AKS/ACI - Add install-connector and remove-connector commands #4996

julienstroheker · 2017-11-29T18:59:41Z

Add az aks install-connector and az aks remove-connector commands

$ az aks install-connector --help

Command
    az aks install-connector: Deploy the ACI-Connector to a AKS cluster.

Arguments
    --name -n           [Required]: Resource name for the managed cluster.
    --name-aks          [Required]: The name of the AKS cluster. The name is case insensitive.
    --resource-group -g [Required]: Name of resource group. You can configure the default group
                                    using `az configure --defaults group=<name>`.
    --chart-url                   : URL to the chart, Default: https://github.com/Azure/aci-
                                    connector-k8s/raw/master/charts/aci-connector.tgz.
    --client-secret               : The secret associated with the service principal. If --service-
                                    principal is specified, then secret should also be specified. If
                                    --service-principal is not specified, the secret is auto-
                                    generated for you and stored in ${HOME}/.azure/ directory.
    --location -l                 : Location. You can configure the default location using `az
                                    configure --defaults location=<location>`.
    --os-type                     : Os type target, could be Windows, Linux or Both.  Default:
                                    Linux.
    --service-principal           : The service principal used for ACI authentication to Azure APIs.
                                    If not specified, it is created for you and stored in the
                                    ${HOME}/.azure directory.

Global Arguments
    --debug                       : Increase logging verbosity to show all debug logs.
    --help -h                     : Show this help message and exit.
    --output -o                   : Output format.  Allowed values: json, jsonc, table, tsv.
                                    Default: json.
    --query                       : JMESPath query string. See http://jmespath.org/ for more
                                    information and examples.
    --verbose                     : Increase logging verbosity. Use --debug for full debug logs.

$ az aks remove-connector --help

Command
    az aks remove-connector: Undeploy the ACI-Connector from an AKS cluster.

Arguments
    --name -n           [Required]: Resource name for the managed cluster.
    --name-aks          [Required]: The name of the AKS cluster. The name is case insensitive.
    --resource-group -g [Required]: Name of resource group. You can configure the default group
                                    using `az configure --defaults group=<name>`.
    --graceful                    : Mention if you want to drain/uncordon your aci-connector to move
                                    your applications running on ACI to your others nodes. Default :
                                    False.
    --os-type                     : Os type target, could be Windows, Linux or Both.  Default:
                                    Linux.

Global Arguments
    --debug                       : Increase logging verbosity to show all debug logs.
    --help -h                     : Show this help message and exit.
    --output -o                   : Output format.  Allowed values: json, jsonc, table, tsv.
                                    Default: json.
    --query                       : JMESPath query string. See http://jmespath.org/ for more
                                    information and examples.
    --verbose                     : Increase logging verbosity. Use --debug for full debug logs.

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

General Guidelines

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).

Command Guidelines

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

(see Authoring Command Modules)

azuresdkci · 2017-11-29T18:59:42Z

View a preview at https://prompt.ws/r/Azure/azure-cli/4996
This is an experimental preview for @microsoft.com users.
(It may take a minute or two for your instance to be ready)
Email feedback to 'azfeedback' with subject 'Prompt Feedback'.

weinong · 2017-11-29T21:32:29Z

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

+    if location is None:
+        location = rgkaci.location  # pylint:disable=no-member
+    # Ensure that the SPN exists
+    principal_obj = _ensure_service_principal(service_principal, client_secret, subscription_id,


I have a concern about it: _ensure_service_principal implicitly grants subscription scope contributor to the spn. I dont think you want it.
Suggest to remove below code from _ensure_service_principal all together as it's not needed:

# add role first before save it if not _add_role_assignment('Contributor', service_principal): logger.warning('Could not create a service principal with the right permissions. ' 'Are you an Owner on this project?')

@weinong You are asking @julienstroheker to remove code thats not even part of the commit. If theres a concern with another module/function i suggest submitting a PR or issue for that code. This is also an overall problem that should be addressed as an issue regarding the functionality of that method. Which I agree, we should not be scoping to an entire sub, but we are already doing that for acs/aks, thus this should be its own issue and un-related to thi pr.

@listonb np! we can fix that separately.

itowlson · 2017-11-29T21:48:44Z

I can't see where the command line options and docs are coming from in the Python, so I'll leave my notes here:

I don't understand the difference between the --name and --name-aks options. Can we draw out this distinction more clearly?
Same comment for --resource-group and --resource-group-name-aks
In uninstall-connector, --gracefull should be --graceful (one l)
In the --linux and --windows options, "Os" should be "OS" (capitalise acronyms)
Why is a target OS required for the 'remove' command?
...that makes me think about the OS switches in general - is it valid to pass both --linux and --windows? If so, the explanation of the OS switches is confusing to me. If not, we should look at changing the design, e.g. to a single --target-os or --os-type option.
--url-chart is documented as "The URL to the chart." Which chart?
Subjective perhaps, but to me "URL chart" reads more like a chart of a URL or URLs. The URL to a chart would be the "chart URL". So perhaps consider changing this option to --chart-url?
In prose, a colon is normally written without a space before it, as in "Default: foo." Is 'space before colon in default value docs' an Azure CLI convention? If not, let's remove the space.

rbitia · 2017-11-29T22:10:34Z

alright so --os for os types with the options windows, linux, both
then we are dropping the new rg for a connector and using the rg they created for the AKS cluster.
--url-chart is for the helm chart if they configured their own we should use --chart-url

rbitia · 2017-11-29T22:12:05Z

Open question about defaults - for simplicity Linux, for coolness Windows + Linux.
Open to debate though.

julienstroheker · 2017-11-29T22:13:12Z

@itowlson Thanks for the comments!

--name is the name of the connector and --name-aks is the name of the cluster where we want to deploy
I will automatically deploy the connector to the same RG where the AKS cluster is deployed, so I will remove the --resource-group-name-aks param.
the graceful typo should be fixed in the last commit
We need to specify the OS type for the remove command for the graceful command, since the we want to drain the nodes and the connector will have a different name depends if you are using windows or/and linux
I will change for ria --os windows --os linux and --os both
I will document the chart_url and applied your comments for the variable naming

itowlson · 2017-11-29T22:25:53Z

@julienstroheker Thanks for the clarifications and for your thoughtful responses. A couple of picky nits around the name and OS stuff, possibly due to my lack of understanding:

At the moment, the --name text says "Resource name for the managed cluster." Will this change to say something like "Resource name for the ACI Connector instance." (or similar)?
I see what you're saying about the OS type in remove, but still don't have the mental model quite straight. Are we saying the the name option is kind of a 'base name' that gets decorated with the OS name to make an actual name? So if I pass --os both then I get two ACI Connectors, and I can remove them separately (e.g. can pass --os linux on remove to drain only the Linux connector)? Pardon me if I'm being slow!

julienstroheker · 2017-11-29T22:39:37Z

@itowlson

I am not sure to understand the question, are you talking about the description in the --help command ? if it the case, right now we have : The name for the ACI Connector but I can change it.
The way that it is coded right now in the connector, if you need windows (--os-type=Windows or --os-type=Both) you have no choice to use the docker image with the tag canary. That image will deploy 1 pod and 2 nodes. Those nodes will have the name aci-connector-0 and aci-connector-1. Since we cannot cleary identify which one is the windows one or the linux one we have no choice to delete both of them. This should be fixed soon, when we will have a better convention name we can apply your scenario.

Make senses ?

robbiezhang · 2017-11-29T22:49:19Z

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

+        try:
+            if windows:
+                drain_node = subprocess.check_output(
+                    ["kubectl", "drain", "aci-connector-0", "--force"],


should use the "name" parameter instead of aci-connector-x

robbiezhang · 2017-11-29T22:56:36Z

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

+                    ["kubectl", "drain", "aci-connector-0", "--force"],
+                    universal_newlines=True)
+                drain_node = subprocess.check_output(
+                    ["kubectl", "drain", "aci-connector-1", "--force"],


same here. show we use {name}-win, {name}-linux as the node name?

This is not supported in the current connector codebase, we cannot specify a custom node name

see @itowlson last comment

itowlson · 2017-11-29T23:30:21Z

@julienstroheker That's cool, I didn't realise the --name text had been updated from the text posted at the top of the PR. Your new text is fine.

Regarding why we need the OS type on remove, thanks for the offline conversation - I'll document it here so others have the information too. The issue, as I understand it, is:

Deployment of the ACI Connector is controlled by a Helm chart. az aks doesn't have low-level control over the nodes that get created.
Depending on whether the OSes include Windows, the Helm chart creates a single aci-connector node (Linux only) or two aci-connector-0/-1 nodes (Linux and Windows). If Helm creates two nodes, it doesn't tell us which is which.
During removal, we could issue drain commands for all these nodes (aci-connector, aci-connector-0 and aci-connector-1) and handle "that node doesn't exist" errors. But relying on node names feels brittle.
Therefore, we prefer the user to tell us what OSes they expect to be removing, and to infer from that what nodes to drain.
In future, it would be nice if the Helm chart labelled the nodes with their OS, or otherwise labelled or annotated them so we could locate them automatically. Then we may be able to remove this requirement.

Let me know any errors or important omissions and I'll edit this comment so it can act as a reference.

…into aci-connector

rbitia · 2017-11-30T17:21:08Z

@troydai not sure who I need from the CLI team to get this merged

listonb · 2017-11-30T17:44:59Z

@devigned @JasonRShaver Can we get a review of this? thx!!

mboersma · 2017-11-30T17:58:49Z

Is it possible to have --name be the name of the AKS managed cluster and --connector-name be the ACI connector's name instead? Then we use --name consistently across all the az aks commands, which would be less surprising to users.

derekbekoe · 2017-11-30T18:38:53Z

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

+    :type chart_url: str
+    :param os_type: Os type target, could be Windows, Linux or Both.
+    :type os_type: str
+    """


All help should be defined in params.py and _help.py not in the docstring of the commands.

--os-type: In params.py, define a choice_list of this param with possible values of ['Windows', 'Linux', 'Both']
--chart-url: In params.py, you can specify the default value as a string default='...'

derekbekoe · 2017-11-30T18:46:03Z

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

+    helm_not_installed = "Error : Helm not detected, please verify if it is installed."
+    image_tag = 'latest'
+    if chart_url is None:
+        url_chart = "https://github.com/Azure/aci-connector-k8s/raw/master/charts/aci-connector.tgz"


If you define chart_url in params.py with a default, this is not needed.

derekbekoe · 2017-11-30T18:47:43Z

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

+    :type os_type: str
+    """
+    from subprocess import PIPE, Popen
+    helm_not_installed = "Error : Helm not detected, please verify if it is installed."


No need for the 'Error: ' prefix.
CLIErrors are already handled by the CLI appropriately (red if color available or with the ERROR: prefix that the logger provides).

derekbekoe · 2017-11-30T18:53:49Z

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

+    profile = Profile()
+    _, _, tenant_id = profile.get_login_credentials()
+    # Check if we want the windows connector
+    if (os_type == 'Windows') or (os_type == 'Both'):


Define 'Windows', 'Both', 'Linux' as constants and this should be a case-insensitive comparison.

derekbekoe · 2017-11-30T18:55:53Z

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

+    :param connector_name: The name for the ACI Connector
+    :type connector_name: str
+    :param graceful: Mention if you want to drain/uncordon your aci-connector to move your applications
+    running on ACI to your others nodes. Default : False


Once you move the param descriptions to params.py, you can define the default there.
Consider making --graceful a flag so if --graceful it set, your graceful variable will be set to true, if not, it will be set to false. This can be done through params.py.

derekbekoe · 2017-11-30T18:56:33Z

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

+    # Just do the get, we don't need the result, it will error out if the group doesn't exist.
+    if graceful:
+        logger.warning('Graceful option selected, will try to drain the node first')
+        kubectl_not_installed = "Error : Kubectl not detected, please verify if it is installed."


No need to prefix with Error :

derekbekoe · 2017-11-30T18:56:56Z

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

+        except OSError:
+            raise CLIError(kubectl_not_installed)
+        try:
+            if (os_type == 'Windows') or (os_type == 'Both'):


As before, should be a case-insensitive comparison.

julienstroheker · 2017-11-30T19:21:48Z

@derekbekoe Thanks for the review ! I am on it !

julienstroheker · 2017-11-30T20:39:56Z

@derekbekoe feel free to look at the new commits. Thanks

derekbekoe

Few more comments.

derekbekoe · 2017-11-30T21:25:29Z

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

+    short-summary: Deploy the ACI-Connector to a AKS cluster.
+    examples:
+        - name: Install the ACI-Connector to an AKS Cluster.
+          text: az aks install-connector --name MyManagedCluster --resource-group MyResourceGroup --connector_name MyConnector


The argument name should be --connector-name not ``--connector_name`.
Same elsewhere in this help file.

derekbekoe · 2017-11-30T21:28:08Z

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

+        - name: Install the ACI-Connector for Windows and Linux to an AKS Cluster.
+          text: az aks install-connector --name MyManagedCluster --resource-group MyResourceGroup --connector_name MyConnector --os-type Both
+        - name: Install the ACI-Connector using specific SPN.
+          text: az aks install-connector --name MyManagedCluster --resource-group MyResourceGroup --connector_name MyConnector --service_principal <SPN_ID> --client_secret<SPN_SECRET>


There would be a space after --client-secret I believe.

derekbekoe · 2017-11-30T21:30:11Z

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

+register_cli_argument('aks install-connector', 'os_type', help='The OS type of the connector', **enum_choice_list(aci_conector_os_type))
+register_cli_argument('aks install-connector', 'chart_url', help='URL to the chart', default=aci_connector_chart_url)
+register_cli_argument('aks remove-connector', 'os_type', help='The OS type of the connector', **enum_choice_list(aci_conector_os_type))
+register_cli_argument('aks remove-connector', 'graceful', help='Mention if you want to drain/uncordon your aci-connector to move your applications', default=False)


For this to actually be a flag, use action='store_true' then remove the default.
Right now, you'd have to specify --graceful True which isn't what is wanted.

derekbekoe · 2017-11-30T21:30:37Z

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

+    :type chart_url: str
+    :param os_type: Os type target.
+    :type os_type: str
+    """


You can delete this whole docstring.

derekbekoe · 2017-11-30T21:30:53Z

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

+    :type graceful: bool
+    :param os_type: Os type target.
+    :type os_type: str
+    """


Can remove this whole docstring.

derekbekoe · 2017-11-30T21:31:42Z

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

@@ -72,6 +72,10 @@ def _get_feature_in_preview_message():

 orchestratorTypes = ["Custom", "DCOS", "Kubernetes", "Swarm", "DockerCE"]

+aci_conector_os_type = ['Windows', 'Linux', 'Both']


nit: typo connector

julienstroheker · 2017-11-30T22:21:09Z

@derekbekoe Thanks again ! I applied your comments.

mboersma

This all looks good to me, my comments are optional. I haven't been able to test it interactively though.

mboersma · 2017-11-30T22:47:05Z

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

+    profile = Profile()
+    _, _, tenant_id = profile.get_login_credentials()
+    # Check if we want the windows connector
+    if (os_type == 'windows') or (os_type == 'both'):


(Optional) you could simplify this a bit:

if os_type in ['windows', 'both']:

I'll do the change, thanks

mboersma · 2017-11-30T22:48:59Z

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

@@ -14,7 +14,7 @@
    logger.warn("Wheel is not available, disabling bdist_wheel hook")
    cmdclass = {}

-VERSION = "2.0.20"
+VERSION = "2.0.21"


I think we're supposed to leave this alone and let the maintainers increment it--at least I had a previous PR where that was the case.

Oh ok, I did it because it failed the CI. Do you want me to put it back version .20?

There is a process change recently that if yours is the first one since the latest release, you should bump up the version here and in the HISTORY.RST; otherwise CI will fail

mboersma · 2017-11-30T22:49:06Z

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

+        except OSError:
+            raise CLIError(kubectl_not_installed)
+        try:
+            if (os_type == 'windows') or (os_type == 'both'):


Same comment as above--could use in here.

I'll do the change too.

yugangw-msft · 2017-11-30T22:47:05Z

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

+    _, browse_path = tempfile.mkstemp()
+    aks_get_credentials(client, resource_group, name, admin=False, path=browse_path)
+    # Validate if the RG exists
+    # Just do the get, we don't need the result, it will error out if the group doesn't exist.


these 2 comments are invalid

yugangw-msft · 2017-11-30T22:50:46Z

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

+    profile = Profile()
+    _, _, tenant_id = profile.get_login_credentials()
+    # Check if we want the windows connector
+    if (os_type == 'windows') or (os_type == 'both'):


Please cross check whether you need to use case-insensitive comparison, considering you have defined the enum list with the first character in capital aci_connector_os_type = ['Windows', 'Linux', 'Both']

yugangw-msft · 2017-11-30T23:01:22Z

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

+                drain_node = subprocess.check_output(
+                    ["kubectl", "drain", "aci-connector-0", "--force"],
+                    universal_newlines=True)
+                drain_node = subprocess.check_output(


suggest use a loop to avoid code dupes, something like

nodes = ['-0', '-1'] if os_type.lower() in ['windows', 'both'] else [''] for n in nodes: drain_node = subprocess.check_output(["kubectl", "drain", "aci-connector{}".format(n), "--force"], universal_newlines=True)

yugangw-msft · 2017-11-30T23:05:12Z

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

+            raise CLIError('Could not find the node, make sure you' +
+                           'are using the correct --os-type option: {}'.format(err))
+        if drain_node:
+            # remove the "pods/" prefix from the name


this comment might be wrong

derekbekoe

LGTM.
Same 2 comments as before. Up to you if you want to address.

derekbekoe · 2017-12-01T00:22:48Z

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

+    --service-principal is not specified, the secret is auto-generated for you
+    and stored in ${HOME}/.azure/ directory.
+    :type client_secret: str
+    """


You can still just remove this block of comments.
Everything should be defined in params.py already.

Got it, let me move all of it in the params.py file to make it cleaner.

derekbekoe · 2017-12-01T00:22:56Z

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

+    :type resource_group: str
+    :param connector_name: The name for the ACI Connector
+    :type connector_name: str
+    """


You can still just remove this block of comments.
Everything should be defined in params.py already.

Got it, let me move all of it in the params.py file to make it cleaner.

yugangw-msft

One more comment. Please do enough manual testing, particularly for both windows and linux, and let me know when it is ready to merge.

yugangw-msft · 2017-12-01T00:48:18Z

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

+            raise CLIError('Could not find the node, make sure you' +
+                           'are using the correct --os-type option: {}'.format(err))
+        if drain_node:
+            drain_node = str(drain_node)


drain_node is not used any more in this method, so why not simplify like

if not drain_node: raise CLIError("Couldn't find the node.")

…4996)

* [AKS] refactor to use Knack framework * AKS/ACI - Add install-connector and remove-connector commands (#4996) * Addressed some review comments. * Use common SSH key validator. * Rename commands loader to avoid a conflict. * Enable tests for the ACS (AKS) module.

Add install-connector and remove-connector cmd

e5c2164

julienstroheker added 3 commits November 29, 2017 13:00

Add the history line

42c21d3

Add --windows and --linux flags

43f248c

Fixing typo

91e7800

derekbekoe added the ACS az acs/aks/openshift label Nov 29, 2017

julienstroheker changed the title ~~[WIP] AKS/ACI - Add install-connector and remove-connector commands~~ AKS/ACI - Add install-connector and remove-connector commands Nov 29, 2017

Typo on command name

ba59b59

weinong reviewed Nov 29, 2017

View reviewed changes

robbiezhang reviewed Nov 29, 2017

View reviewed changes

julienstroheker added 5 commits November 29, 2017 17:31

Applying review comments.

d3f90b1

Merge branch 'dev' into aci-connector

0dd4492

lint

f813da0

Merge branch 'aci-connector' of github.com:julienstroheker/azure-cli …

6d8cbbf

…into aci-connector

lint

1d5f2a1

Changing variables names

ad27361

derekbekoe self-requested a review November 30, 2017 18:49

derekbekoe suggested changes Nov 30, 2017

View reviewed changes

julienstroheker added 2 commits November 30, 2017 14:35

applied review

23b0dea

applied review

09fb6ba

lint

64e1a8c

derekbekoe suggested changes Nov 30, 2017

View reviewed changes

Apply review

b36c14b

mboersma approved these changes Nov 30, 2017

View reviewed changes

yugangw-msft reviewed Nov 30, 2017

View reviewed changes

Applied review

15d32e5

derekbekoe approved these changes Dec 1, 2017

View reviewed changes

Move all the help in params.py

372cca4

yugangw-msft reviewed Dec 1, 2017

View reviewed changes

clean CLIError method

3ee223f

yugangw-msft merged commit c360569 into Azure:dev Dec 1, 2017

mboersma mentioned this pull request Dec 4, 2017

[AKS] refactor to use Knack framework #5036

Merged

mboersma pushed a commit to mboersma/azure-cli that referenced this pull request Dec 7, 2017

AKS/ACI - Add install-connector and remove-connector commands (Azure#…

d47e1f1

…4996)

haroldrandom added the ACS az acs/aks/openshift label Oct 25, 2019

		@@ -72,6 +72,10 @@ def _get_feature_in_preview_message():

		orchestratorTypes = ["Custom", "DCOS", "Kubernetes", "Swarm", "DockerCE"]

		aci_conector_os_type = ['Windows', 'Linux', 'Both']

AKS/ACI - Add install-connector and remove-connector commands #4996

AKS/ACI - Add install-connector and remove-connector commands #4996

Conversation

julienstroheker commented Nov 29, 2017 • edited Loading

General Guidelines

Command Guidelines

azuresdkci commented Nov 29, 2017

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

itowlson commented Nov 29, 2017

rbitia commented Nov 29, 2017 • edited Loading

rbitia commented Nov 29, 2017

julienstroheker commented Nov 29, 2017

itowlson commented Nov 29, 2017

julienstroheker commented Nov 29, 2017

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

itowlson commented Nov 29, 2017

rbitia commented Nov 30, 2017 • edited Loading

listonb commented Nov 30, 2017

mboersma commented Nov 30, 2017 • 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

julienstroheker commented Nov 30, 2017

julienstroheker commented Nov 30, 2017

derekbekoe 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienstroheker commented Nov 30, 2017

mboersma 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

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 Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbekoe 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

Choose a reason for hiding this comment

yugangw-msft left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienstroheker commented Nov 29, 2017 •

edited

Loading

rbitia commented Nov 29, 2017 •

edited

Loading

rbitia commented Nov 30, 2017 •

edited

Loading

mboersma commented Nov 30, 2017 •

edited

Loading

yugangw-msft Nov 30, 2017 •

edited

Loading

yugangw-msft left a comment •

edited

Loading