sf ARM cli #4132

QingChenmsft · 2017-08-03T17:30:13Z

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.

msftclas · 2017-08-03T17:30:16Z

@QingChenmsft,
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

derekbekoe · 2017-08-11T19:12:47Z

azure-cli2017.pyproj

@@ -14,8 +14,7 @@
    <LaunchProvider>Standard Python launcher</LaunchProvider>
    <InterpreterId>MSBuild|{54f4b6dc-0859-46dc-99bb-b275c9d0aca3}|$(MSBuildProjectFullPath)</InterpreterId>
    <EnableNativeCodeDebugging>False</EnableNativeCodeDebugging>
-    <CommandLineArguments>
-    </CommandLineArguments>
+    <CommandLineArguments>sf cluster new -g tcli5 -l westus --cluster-size 1 --certificate-subject-name test.com --certificate-output-folder c:\test --vm-password User@123456789</CommandLineArguments>


Remove this from the project file.

derekbekoe · 2017-08-11T19:13:24Z

src/azure-cli-core/azure/cli/core/commands/__init__.py

@@ -378,7 +378,7 @@ def get_command_table(module_name=None):
    If the module is not found, all commands are loaded.
    '''
    loaded = False
-    if module_name and module_name not in BLACKLISTED_MODS:
+    if module_name and module_name not in BLACKLISTED_MODS and module_name != 'sf':


Add a TODO to remove this later.
Would you remove once the old module has been deprecated?

derekbekoe · 2017-08-11T19:14:51Z

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

+        - name: Specify an existing Certificate resource in a key vault and a custom template to deploy a cluster
+          text: >
+            az sf cluster new -g group-name -n cluster1 -l westus --template-file c:\\template.json --parameter-file c:\\parameter.json 
+            --secret-identifier https://chackokv1.vault.azure.net:443/secrets/chackdantestcertificate4/56ec774dc61a462bbc645ffc9b4b225f


This points to a real keyvault... would suggest something else.

This still looks like real data rather than example data.

derekbekoe · 2017-08-11T19:17:11Z

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

+                      help='The password of the certificate file.')
+register_cli_argument('sf', 'certificate_subject_name',options_list=('--certificate-subject-name', '-sn','-csn'),
+                      help='The subject name of the certificate to be created.')
+register_cli_argument('sf', 'vault_resource_group_name',options_list=('--vault-resource-group', '-vg','-vrg'),


This looks really strange. 3 options for the same argument?
Also, in the CLI I've only seen short form options that have a single char not 2 or 3.

Yes, short options must only be a single character. If they are multi-character, they should be preceded by --. And it is highly unusually to have multiple long-options in the CLI. Only when one is going to be deprecated in a future release.

remove other 2.

derekbekoe · 2017-08-11T19:17:30Z

src/command_modules/azure-cli-servicefabric/azure/cli/command_modules/servicefabric/_utils.py

+
+from os.path import exists, join
+from OpenSSL import crypto
+


Nothing in this file.

derekbekoe · 2017-08-11T19:20:02Z

...es/azure-cli-servicefabric/azure/cli/command_modules/servicefabric/tests/test_sf_commands.py

+class ServiceFabricTest(ScenarioTest):
+    def test_list_storage_account(self):
+        cluster_list = self.cmd('az sf cluster list').get_output_in_json()
+        assert len(cluster_list) > 0


Need more tests than this.
There are several new commands added and lots of custom logic to be tested.

Not only should you cover all of your new commands, you should exercise as many of your parameters as possible. There are many parameter registrations that have nothing more than help='Text..'. Usually more kwargs are necessary to support relevant scenarios.

derekbekoe · 2017-08-11T19:20:44Z

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

+        'azure.cli.command_modules.servicefabric'
+    ],
+    install_requires=DEPENDENCIES,
+)


See https://github.com/Azure/azure-cli/blob/master/doc/authoring_command_modules/example_module_template/setup.py on how this should be structured.

tjprescott

Test coverage is inadequate. We need to see the <command> -h output for each of the "interesting" commands (creates, updates, adds). I would recommend setting up a quick meeting to go over your command tree structure.

tjprescott · 2017-08-11T19:32:31Z

azure-cli2017.pyproj

@@ -529,7 +547,6 @@
    <Compile Include="command_modules\azure-cli-component\azure\cli\command_modules\component\custom.py" />
    <Compile Include="command_modules\azure-cli-component\azure\cli\command_modules\component\commands.py" />
    <Compile Include="command_modules\azure-cli-component\azure\cli\command_modules\component\_params.py" />
-    <Compile Include="command_modules\azure-cli-network\azure\cli\command_modules\network\custom.py" />


Revert this change.

This still is showing as being removed.

ok, should be fixed now.

tjprescott · 2017-08-11T19:33:43Z

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

+    type: command
+    short-summary: Add a secondary cluster certificate to the cluster.
+    examples:
+        - name: Example1


Needs more descriptive name than "Example1" "Example2"...

Still says Example1, Example2...

tjprescott · 2017-08-11T19:35:16Z

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

+# PARAMETER REGISTRATIONS
+
+register_cli_argument('sf cluster list', 'resource_group_name', resource_group_name_type,
+                      id_part=None, required=False, help='The resouce group name')


"requiredness" is inferred from the SDK or custom command signature and does not usually need to be specified here unless the SDK is wrong for some reason.

tjprescott · 2017-08-11T19:36:40Z

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

+
+register_cli_argument('sf', 'resource_group_name', resource_group_name_type,
+                      id_part=None, required=True, help='The resouce group name')
+register_cli_argument('sf', 'cluster_name',options_list=('--name', '--cluster-name','-n'),


This should be registered as name_type. I'm okay with having both --name and --cluster-name, although that is uncommon in the CLI.

keep it same as our powershell

tjprescott · 2017-08-11T19:37:30Z

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

+register_cli_argument('sf', 'location',
+                      validator=get_default_location_from_resource_group)
+
+register_cli_argument('sf', 'secret_identifier',options_list=('--secret-identifier', '-si'),


Short options must be a single letter. Err on the side of no short options.

tjprescott · 2017-08-11T19:48:17Z

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

+
+cli_command(__name__, 'sf cluster reliability update', custom_path.format('update_cluster_reliability_level'), factory, no_wait_param=False)
+
+cli_command(__name__, 'sf cluster durability update', custom_path.format('update_cluster_durability'), factory, no_wait_param=False)


You should not have groups (reliability/durability) with only a single command. These could easily be folded into parameters for a sf cluster update command (which is missing BTW) as --reliability-level and --durability-level respectively.

all the command are related to cluster update, we think keeping them this way, which makes user more clear.

tjprescott · 2017-08-11T19:48:49Z

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

+
+cli_command(__name__, 'sf cluster durability update', custom_path.format('update_cluster_durability'), factory, no_wait_param=False)
+
+cli_command(__name__, 'sf cluster node-type add', custom_path.format('add_cluster_node_type'), factory, no_wait_param=False)


There is no node-type remove command.

this is a bug in our service, we can't have this command ready for now. we will add when we have resolved the issue.

tjprescott · 2017-08-11T19:49:44Z

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

+
+cli_command(__name__, 'sf cluster node remove', custom_path.format('remove_cluster_node'), factory, no_wait_param=False)
+
+cli_command(__name__, 'sf cluster upgrade-type set', custom_path.format('update_cluster_upgrade_type'), factory, no_wait_param=False)


This should also be folded into a sf cluster update command as a parameter. Additionally, whatever parameters are exposed on update should (if feasible) be supported on create as well.

do not understand, do you mean "'sf cluster upgrade-type set'" should be "sf cluster update upgrade-type"?

I would need more context on your service, but this looks like an entire command to update one parameter, which should not be done. If upgrade-type is a property of a cluster, then there should be a sf cluster update command with --upgrade-type as a parameter.

tjprescott · 2017-08-11T19:50:49Z

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

+
+cli_command(__name__, 'sf cluster upgrade-type set', custom_path.format('update_cluster_upgrade_type'), factory, no_wait_param=False)
+
+cli_command(__name__, 'sf application certificate add', custom_path.format('add_app_cert'), factory, no_wait_param=False)


This is a subcommand with a single subcommand with a single command! Are there going to be more sf application commands? Are there going to be more sf application certificate commands? Why are there no plain sf commands?

yes, there will be more for application commands.

tjprescott · 2017-08-11T19:54:06Z

...es/azure-cli-servicefabric/azure/cli/command_modules/servicefabric/tests/test_sf_commands.py

+class ServiceFabricTest(ScenarioTest):
+    def test_list_storage_account(self):
+        cluster_list = self.cmd('az sf cluster list').get_output_in_json()
+        assert len(cluster_list) > 0


Not only should you cover all of your new commands, you should exercise as many of your parameters as possible. There are many parameter registrations that have nothing more than help='Text..'. Usually more kwargs are necessary to support relevant scenarios.

tjprescott

There still seem to be numerous issues.

tjprescott · 2017-08-24T15:21:07Z

azure-cli2017.pyproj

@@ -529,7 +547,6 @@
    <Compile Include="command_modules\azure-cli-component\azure\cli\command_modules\component\custom.py" />
    <Compile Include="command_modules\azure-cli-component\azure\cli\command_modules\component\commands.py" />
    <Compile Include="command_modules\azure-cli-component\azure\cli\command_modules\component\_params.py" />
-    <Compile Include="command_modules\azure-cli-network\azure\cli\command_modules\network\custom.py" />


This still is showing as being removed.

tjprescott · 2017-08-24T15:22:35Z

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

@@ -0,0 +1,4 @@
+0.0.1 (2017-8-23)


Should just say "unreleased"

tjprescott · 2017-08-24T15:23:15Z

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

+    short-summary: List all the cluster resource in the same subscription
+"""
+
+helps["sf cluster new"] = """


This should be sf cluster create not new.

we want to keep aligned with PowerShell which is New

This will not be accepted. There are 0 "new" commands in the CLI.

okay, i will update, yesterday, i forgot to push the change, you can take a look, i just pushed.

tjprescott · 2017-08-24T15:23:55Z

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

+        - name: Specify an existing Certificate resource in a key vault and a custom template to deploy a cluster
+          text: >
+            az sf cluster new -g group-name -n cluster1 -l westus --template-file c:\\template.json --parameter-file c:\\parameter.json 
+            --secret-identifier https://chackokv1.vault.azure.net:443/secrets/chackdantestcertificate4/56ec774dc61a462bbc645ffc9b4b225f


This still looks like real data rather than example data.

tjprescott · 2017-08-24T15:24:10Z

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

+    type: command
+    short-summary: Add a secondary cluster certificate to the cluster.
+    examples:
+        - name: Example1


Still says Example1, Example2...

tjprescott · 2017-08-24T15:29:35Z

...es/azure-cli-servicefabric/azure/cli/command_modules/servicefabric/tests/test_sf_commands.py

+from azure.cli.testsdk.vcr_test_base import (ResourceGroupVCRTestBase,
+                                             JMESPathCheck, NoneCheck)
+
+class ServiceFabricNewClusterTest(ResourceGroupVCRTestBase):


This is an older test base--these should really use the ScenarioTest base. Don't change them for this PR, but FYI for the future.

tjprescott · 2017-08-24T15:31:12Z

...es/azure-cli-servicefabric/azure/cli/command_modules/servicefabric/tests/test_sf_commands.py

+                 checks=[JMESPathCheck('clientCertificateThumbprints[0].certificateThumbprint', test_thumbprint),
+                     JMESPathCheck('clientCertificateThumbprints[0].isAdmin', False)])
+
+        self.cmd('sf cluster client-certificate remove --resource-group {} --name {} --thumbprints {}'.format(rg,name,test_thumbprint), 


This needs to test the multi-thumbprint scenario, as I have doubts that it will work the way you have things registered.

tjprescott · 2017-08-24T15:33:54Z

...es/azure-cli-servicefabric/azure/cli/command_modules/servicefabric/tests/test_sf_commands.py

+        self.cmd('sf application certificate add --resource-group {} --cluster-name {} --certificate-subject-name test2.com'.format(rg,name),
+                           checks=NoneCheck())
+
+if __name__ == '__main__':


I might have missed some, but it looks like tests are missing for the following commands:
sf cluster setting remove, sf cluster durability update, sf cluster node-type add, sf cluster node add/remove

tjprescott · 2017-08-24T15:38:50Z

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

+register_cli_argument('sf cluster client-certificate', 'certificate_common_name',
+                      help='Specify client certificate common name.')
+register_cli_argument('sf cluster client-certificate', 'admin_client_thumbprints',
+                      help='Specify client certificate thumbprint that only has admin permission.')


Remove "Specify..." from these entries.

tjprescott · 2017-08-24T15:39:47Z

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

+register_cli_argument('sf client certificate', 'certificate_common_name',
+                      help='Specify client certificate common name.')
+register_cli_argument('sf client certificate', 'admin_client_thumbprints',
+                      help='Specify list of client certificate thumbprint that only has admin permission.')


How do you specify the list? The only valid lists in the CLI are space-separated lists, and the help should call this out.

derekbekoe · 2017-08-24T17:33:05Z

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

+from ._client_factory import keyvault_client_factory
+from ._client_factory import compute_client_factory
+from ._client_factory import storage_client_factory
+from ._client_factory import network_client_factory


from ._client_factory import (resource_client_factory, keyvault_client_factory, ...)

will update.

lmazuel · 2017-08-24T18:33:38Z

@tjprescott @QingChenmsft https://pypi.python.org/pypi/azure-mgmt-servicefabric/0.1.0

tjprescott · 2017-08-25T16:19:19Z

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

-register_cli_argument('sf cluster client-certificate remove', 'thumbprint',
-                      help='a single or list of client certificate thumbprint(s) to be remove.')
+register_cli_argument('sf cluster client-certificate remove', 'thumbprints', nargs='+',
+                      help='A single or Space separated list of client certificate thumbprint(s) to be remove.')


You can just say "Space separated list of ...". That's the common pattern in the CLI and no one has ever complained that the didn't realize they could use just one. :)

tjprescott · 2017-08-25T16:20:20Z

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

-               """for example: [{"section": "NamingService","parameter": "MaxOperationTimeout","value": 1000},{"section": "MaxFileOperationTimeout","parameter": "Max2","value": 1000]""")
+register_cli_argument('sf cluster setting set', 'settings_section_description', type=get_json_object,
+                      help='JSON encoded parameters configuration. Use @{file} to load from a file.'
+                      """for example: [{"section": "NamingService","parameter": "MaxOperationTimeout","value": 1000},{"section": "MaxFileOperationTimeout","parameter": "Max2","value": 1000]""")  # pylint: disable=line-too-long


This would be more appropriate for the parameter long-summary in the helps.py file.

tjprescott · 2017-08-25T16:21:49Z

...es/azure-cli-servicefabric/azure/cli/command_modules/servicefabric/tests/test_sf_commands.py


-        self.cmd('sf cluster list', checks=[JMESPathCheck('type(@)', 'array')])
+        print('####testing list####')


Please remove these print statements.

tjprescott

Much closer. Still a few commands missing tests, and please remove the print statements in the tests. Otherwise it's little help doc nits that can easily be done in follow-up PRs.

tjprescott · 2017-08-25T16:23:29Z

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

-               """for example: [{"section": "NamingService","parameter": "MaxOperationTimeout"}]""")
+register_cli_argument('sf cluster setting remove', 'settings_section_description', type=get_json_object,
+                      help='JSON encoded parameters configuration. Use @{file} to load from a file.'
+                      """for example: [{"section": "NamingService","parameter": "MaxOperationTimeout"}]""")


Why is a JSON object needed for a remove operation?

tjprescott · 2017-08-25T16:24:05Z

...es/azure-cli-servicefabric/azure/cli/command_modules/servicefabric/tests/test_sf_commands.py

+                 checks=[JMESPathCheck('nodeTypes[0].durabilityLevel', durability_level)])
+
+        print('####testing add app cert###')
+        self.cmd('sf application certificate add --resource-group {} --cluster-name {} --certificate-subject-name test2.com'.format(rg, name),


Looks like node-type add and node add/remove are still missing.

derekbekoe · 2017-08-25T17:38:02Z

** Please check this:

The .json files you've included, should they be shipped as part of your package or are they only for tests? (e.g. azure/cli/command_modules/servicefabric/template/linux/template.json).

If they are required as part of the shipped package, include them e.g. https://github.com/Azure/azure-cli/blob/master/src/command_modules/azure-cli-acr/setup.py#L61.

To verify, run python setup.py bdist_wheel from the directory of your command module. Then open/extract the generated .whl file and confirm that your JSON files are in there.

QingChenmsft · 2017-08-25T17:39:45Z

Hi Derek,
Thanks, can you give me a example how to add this in setup.py ? then I will try to verify it laster.

derekbekoe · 2017-08-25T17:41:05Z

@QingChenmsft I included an example in my message above. :)

codecov-io · 2017-08-26T00:49:46Z

Codecov Report

Merging #4132 into master will decrease coverage by 0.96%.
The diff coverage is 43.26%.

@@            Coverage Diff             @@
##           master    #4132      +/-   ##
==========================================
- Coverage      70%   69.03%   -0.97%     
==========================================
  Files         475      484       +9     
  Lines       29919    31039    +1120     
  Branches     4627     4850     +223     
==========================================
+ Hits        20945    21429     +484     
- Misses       7490     8030     +540     
- Partials     1484     1580      +96

Impacted Files	Coverage Δ
...azure-cli-core/azure/cli/core/commands/__init__.py	`69.82% <100%> (ø)`	⬆️
...c/azure/cli/command_modules/servicefabric/_help.py	`100% <100%> (ø)`
..._modules/azure-cli-servicefabric/azure/__init__.py	`100% <100%> (ø)`
...zure/cli/command_modules/servicefabric/__init__.py	`100% <100%> (ø)`
...zure/cli/command_modules/servicefabric/commands.py	`100% <100%> (ø)`
...ervicefabric/azure/cli/command_modules/__init__.py	`100% <100%> (ø)`
...ules/azure-cli-servicefabric/azure/cli/__init__.py	`100% <100%> (ø)`
...azure/cli/command_modules/servicefabric/_params.py	`100% <100%> (ø)`
.../azure/cli/command_modules/servicefabric/custom.py	`36.1% <36.1%> (ø)`
...i/command_modules/servicefabric/_client_factory.py	`76.92% <76.92%> (ø)`
... and 13 more

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 4d67155...38e5e6d. Read the comment docs.

derekbekoe · 2017-08-28T17:29:51Z

@tjprescott to take a look also.

tjprescott

Approved with outstanding high-priority issues described in #4328. Futures PRs except for bug fixes to the existing commands (i.e. new commands or new features to commands) will be blocked until #4328 is resolved.

msftclas added the cla-not-required label Aug 3, 2017

derekbekoe suggested changes Aug 11, 2017

View reviewed changes

tjprescott suggested changes Aug 11, 2017

View reviewed changes

QingChenmsft mentioned this pull request Aug 21, 2017

sf arm py sdk Azure/azure-sdk-for-python#1346

Merged

QingChenmsft force-pushed the master_test branch from 14998ec to 879be6f Compare August 23, 2017 22:33

tjprescott suggested changes Aug 24, 2017

View reviewed changes

derekbekoe reviewed Aug 24, 2017

View reviewed changes

tjprescott reviewed Aug 25, 2017

View reviewed changes

tjprescott suggested changes Aug 25, 2017

View reviewed changes

QingChenmsft force-pushed the master_test branch from 86b043c to f1aeef5 Compare August 28, 2017 00:11

service fabric arm cli

e5137c6

QingChenmsft force-pushed the master_test branch from 321ef03 to e5137c6 Compare August 28, 2017 01:26

fix

38e5e6d

derekbekoe approved these changes Aug 28, 2017

View reviewed changes

tjprescott mentioned this pull request Aug 28, 2017

[ServiceFabric] Blocking Issues for SF command GA #4328

Open

3 tasks

tjprescott approved these changes Aug 28, 2017

View reviewed changes

tjprescott merged commit 8d24b5a into Azure:master Aug 28, 2017

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


		cli_command(__name__, 'sf cluster reliability update', custom_path.format('update_cluster_reliability_level'), factory, no_wait_param=False)

		cli_command(__name__, 'sf cluster durability update', custom_path.format('update_cluster_durability'), factory, no_wait_param=False)


		cli_command(__name__, 'sf cluster durability update', custom_path.format('update_cluster_durability'), factory, no_wait_param=False)

		cli_command(__name__, 'sf cluster node-type add', custom_path.format('add_cluster_node_type'), factory, no_wait_param=False)


		cli_command(__name__, 'sf cluster node remove', custom_path.format('remove_cluster_node'), factory, no_wait_param=False)

		cli_command(__name__, 'sf cluster upgrade-type set', custom_path.format('update_cluster_upgrade_type'), factory, no_wait_param=False)


		cli_command(__name__, 'sf cluster upgrade-type set', custom_path.format('update_cluster_upgrade_type'), factory, no_wait_param=False)

		cli_command(__name__, 'sf application certificate add', custom_path.format('add_app_cert'), factory, no_wait_param=False)


		self.cmd('sf cluster list', checks=[JMESPathCheck('type(@)', 'array')])
		print('####testing list####')

sf ARM cli #4132

sf ARM cli #4132

Conversation

QingChenmsft commented Aug 3, 2017 • edited by tjprescott Loading

General Guidelines

Command Guidelines

msftclas commented Aug 3, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott 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

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

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

tjprescott 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

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

lmazuel commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbekoe commented Aug 25, 2017

QingChenmsft commented Aug 25, 2017

derekbekoe commented Aug 25, 2017

codecov-io commented Aug 26, 2017 • edited Loading

QingChenmsft commented Aug 3, 2017 •

edited by tjprescott

Loading

codecov-io commented Aug 26, 2017 •

edited

Loading