Adding support for Service Fabric client commands #3136

samedder · 2017-05-02T20:04:35Z

This PR introduces the following set of changes

New command module designed to manage Service Fabric clusters once they have been provisioned. This includes managing the cluster entities such as nodes and applications
New preparer for scenario tests that require an active cluster connection
Small set of unit tests that cover basic commands that do not actively alter the state of the Service Fabric cluster

msftclas · 2017-05-02T20:04:40Z

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

samedder · 2017-05-02T20:37:07Z

CI is expected to fail as this depends on a package that is not currently publicly available.

troydai · 2017-05-03T21:24:42Z

CI will fail because of the code style as well. Please fix these errors first. Run the following command first:

check_style --module sf --pep8 --pylint

troydai · 2017-05-03T21:25:19Z

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

+
+from azure.cli.core.help_files import helps
+
+# pylint: disable=line-too-long


troydai · 2017-05-03T21:27:02Z

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

+# Custom commands
+
+# cluster
+cli_command(__name__, "sf cluster select", "azure.cli.command_modules.sf.custom#sf_select")


Since your parameters definition uses the context module, the command's definition could adopt that pattern, too. Reference: https://github.com/Azure/azure-cli/blob/master/src/command_modules/azure-cli-sql/azure/cli/command_modules/sql/commands.py

troydai · 2017-05-03T21:28:22Z

src/command_modules/azure-cli-sf/tests/test_sf_commands.py

@@ -0,0 +1,154 @@
+import os
+
+from azure.cli.testsdk import ScenarioTest, JMESPathCheck, \


from azure.cli.testsdk import (ScenarioTest, JMESPathCheck, JMESPathCheckExists, NoneCheck)

troydai · 2017-05-03T21:29:53Z

src/command_modules/azure-cli-sf/tests/test_sf_commands.py

+
+    def create_resource(self, _, **kwargs):
+        # Omit name here since there is no randomization required
+        endpoint = os.environ.get(self.env_variable_name, self.endpoint)


This will make the test unrunnable in CI unless a specific environment is set. We want to avoid special setting for one particular group of test otherwise parallel run is prohibited. Can this endpoint be retrieved by other means?

So unfortunately, unlike mgmt command modules our tests require that whoever makes the change provisions a Service Fabric cluster, and then runs this small set of tests. Hence, the use of the environment variable since right now we cannot provision a cluster from the CLI for every CI test, it would take far too long.

We could make these manual tests and omit them from CI, telling users if they want to modify the sf commands, they need to provision a cluster then run these tests.

I'm not sure what's best here, unfortunately we don't have a good light weight testing mechanism since a these commands end up exercising long code paths in the Service Fabric clusters.

troydai · 2017-05-03T21:30:26Z

src/command_modules/azure-cli-sf/tests/test_sf_commands.py

+    def create_resource(self, _, **kwargs):
+        # Omit name here since there is no randomization required
+        endpoint = os.environ.get(self.env_variable_name, self.endpoint)
+        print("endpoint {}".format(endpoint))


Use logger instead. Stdout is usually suppressed in automation.

troydai · 2017-05-03T21:47:18Z

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

+            t = os.stat(os.path.join(root, file))
+            total_files_size += t.st_size
+
+    def print_progress(size, rel_file_path):


At least offer user options to run this command quietly. Respect the Rule of Silence (http://www.linfo.org/rule_of_silence.html)

troydai · 2017-05-03T21:50:15Z

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

+    import ApplicationCapacityDescription
+    from azure.servicefabric.models.application_metric_description import \
+    ApplicationMetricDescription
+    from azure.cli.command_modules.sf._factory import cf_sf_client


Best practice: http://docs.python-guide.org/en/latest/writing/style/#line-continuations

troydai · 2017-05-03T21:56:41Z

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

+    import ServicePlacementRequireDomainDistributionPolicyDescription
+
+    r = None
+    if formatted_placement_policies is not None:


I'd like to make sure this is your intended behavior here:

formatted_placement_policies is None, function return None

len(formatted_placement_policies) == 0, function return [] and empty list

return a fulfilled list

Remember the expression bool([]) returns False, which offers a more useful check on list than is not None.

Ususally, I would write this function in a simpler manner:

if formatted_placement_policies: r = [] # other logic return r else: return None # or []

troydai · 2017-05-03T22:01:13Z

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

+        f += 512
+    return f
+
+def sf_create_service(app_id, name, type, stateful=False, stateless=False, # pylint: disable=R0913


pylint: disable=too-many-arguments
for better readability

troydai · 2017-05-03T22:04:38Z

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

+            "ExclusiveProcess"
+        ]
+        if activation_mode not in valid_modes:
+            raise CLIError("Invalid activation mode specified")


if activation_mode not in ('SharedProcess', 'ExclusiveProcess`, None): raise CLIError( # ... )

derekbekoe · 2017-05-03T22:39:41Z

@samedder Any update on this PR?

samedder · 2017-05-04T02:00:50Z

@derekbekoe and @troydai thanks for the review. The only issue I don't know how to address well is the testing side.

I'll push an update later tonight.

samedder · 2017-05-04T12:33:44Z

New changes to address comments, should also be passing PEP8 and PyLint now. Let me know what else needs to be changed. I think the only issue I have outstanding is how to do the testing. @troydai , I just renamed the files for the time being so they won't be used in CI.

troydai · 2017-05-04T16:09:03Z

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

+        total_files_count += (len(files) + 1)
+        for file in files:
+            t = os.stat(os.path.join(root, file))
+            total_files_size += t.st_size


It seems to be that progress report is removed. Therefore the total file size and count are not used anywhere. Am I right?

I added the logic to display file upload progress but only on --verbose. Is that okay or should we include maybe a --show-progress flag?

Two things to be done regarding the progress indicator:

use a --show-progress, --version has its own meaning, it is associated with elevating logging level.

print the progress information to stderr instead of stdout, as other Unix tools do.

troydai · 2017-05-04T16:09:37Z

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

+                                              rel_path, file))
+            ).replace("\\", "/")
+            # pylint: disable=line-too-long
+            with open(os.path.normpath(os.path.join(root, file)), 'rb') as file_opened:  # noqa: justification, no way to shorten


You can move the path expression out of the open function to shorten the line.

troydai · 2017-05-04T16:12:51Z

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

+        ServicePlacementRequireDomainDistributionPolicyDescription
+    )
+
+    if formatted_placement_policies:


Thanks for making the changes. However, there are multiple similar cases in this module which I didn't comment on. Do they suit the same fixes?

They do, I changed it so we treat [] and None the same and return None

troydai · 2017-05-04T16:13:15Z

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

+
+
+# pylint: disable=too-many-arguments
+# pylint: disable=R0915


Duplication.

troydai · 2017-05-04T17:05:56Z

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

+
+    if activation_mode is not None:
+        if activation_mode not in ["SharedProcess", "ExclusiveProcess"]:
+            raise CLIError("Invalid activation mode specified")


Isn't this a more concise form?

if activation_mode not in ('SharedProcess', 'ExclusiveProcess`, None): raise CLIError( # ... )

troydai · 2017-05-04T17:06:12Z

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

+                "Cannot specify standby replica keep duration for stateless "
+                "service"
+            )
+        # pylint: disable=R0204


use the readable rule name.

troydai · 2017-05-04T17:07:30Z

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

+def sf_report_partition_health(partition_id, source_id, health_property,
+                               health_state, ttl=None, description=None,
+                               sequence_number=None, remove_when_expired=None,
+                               timeout=60):


Move the pylint disable argument in line. This way the disable statement won't leak into the file scope.

def sf_report_partition_health(partition_id, # pylint: disable=too-many-arguments source_id, health_property, health_state, ttl=None, description=None, sequence_number=None, remove_when_expired=None, timeout=60):

For other places as well.

samedder · 2017-05-04T20:27:13Z

One more question, should we do a rebase squash before the final merge for this?

troydai

Looks good overall as long as the progress indicator is addressed. Please also fix the CI.

troydai · 2017-05-04T21:46:34Z

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

+    def print_progress(size, rel_file_path):
+        nonlocal current_file_size
+        current_file_size += size
+        logger.info("[%d/%d] files, [%d/%d] bytes, %s", current_files_count,


Print to stderr

samedder · 2017-05-04T23:53:02Z

Changed the behavior to print to stderr, let me know if this is unexpected. The python package now also exists on PyPi.

johanste · 2017-05-05T00:48:37Z

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

+with open('HISTORY.rst', 'r', encoding='utf-8') as f:
+    HISTORY = f.read()
+
+setup(


Please make sure that you are using the azure_bdist_wheel.py file to exclude the azure/init.py, azure/cli/init.py, azure/cli/command_modules/init.py files (see the setup.py/setup.cfg for any of the other command packages)

Ah thanks, that seems to be the issue. I see the bdist_wheel after pulling the latest changes.

troydai · 2017-05-05T01:20:13Z

Fix CI.

Code style
Files without licenses

samedder · 2017-05-05T02:06:49Z

@troydai can you help us figure out why the the python3 builds are failing with missing licenses? I don't see any changes that could be related to this.

troydai · 2017-05-05T02:16:44Z

Because the headers you added are not the exact match:

azure-cli/scripts/license/_common.py

Line 15 in c1684ea

"""

It will fall on all platforms. It shows on 3.6 only because the build fail on other two platforms before headers are checked. As I repeatedly emphasize throughout this pull request, please run check_style --ci or check_style --module sf --pep8 --pylint before pushing your changes. It is quicker to discover issues locally.

samedder · 2017-05-05T02:19:23Z

@troydai I do :) every time. I just found that bug as well. I see what I need to change now.
I promise I'm not cheating

(env) E:\work_root\azure\cli>git reset --hard
HEAD is now at 02e97a90 Adding ServiceFabric commands for version 5.6

(env) E:\work_root\azure\cli>.\scripts\check_style --module sf --pep8 --pylint


Run flake8 for PEP8 compliance
Modules: sf
Flake8 passed


Run pylint
Modules: sf
Pylint passed

troydai · 2017-05-05T02:22:55Z

You may need to clean up your workspace and run on both python 2 and 3.

git clean ./src -xdff

codecov-io · 2017-05-05T04:40:57Z

Codecov Report

Merging #3136 into master will decrease coverage by 0.98%.
The diff coverage is 26.72%.

@@            Coverage Diff             @@
##           master    #3136      +/-   ##
==========================================
- Coverage   71.12%   70.14%   -0.99%     
==========================================
  Files         362      371       +9     
  Lines       23929    24586     +657     
  Branches     3658     3767     +109     
==========================================
+ Hits        17019    17245     +226     
- Misses       5812     6237     +425     
- Partials     1098     1104       +6

Impacted Files	Coverage Δ
...command_modules/azure-cli-sf/azure/cli/__init__.py	`100% <100%> (ø)`
src/command_modules/azure-cli-sf/azure/__init__.py	`100% <100%> (ø)`
...azure-cli-sf/azure/cli/command_modules/__init__.py	`100% <100%> (ø)`
...re-cli-sf/azure/cli/command_modules/sf/__init__.py	`100% <100%> (ø)`
...re-cli-sf/azure/cli/command_modules/sf/commands.py	`100% <100%> (ø)`
...ure-cli-sf/azure/cli/command_modules/sf/_params.py	`100% <100%> (ø)`
...azure-cli-sf/azure/cli/command_modules/sf/_help.py	`100% <100%> (ø)`
...re-cli-sf/azure/cli/command_modules/sf/_factory.py	`5.55% <5.55%> (ø)`
...zure-cli-sf/azure/cli/command_modules/sf/custom.py	`9.15% <9.15%> (ø)`
...urce/azure/cli/command_modules/resource/_params.py	`100% <0%> (ø)`	⬆️
... 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 49c281b...29053f3. Read the comment docs.

yugangw-msft · 2017-05-05T16:49:54Z

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

+DEPENDENCIES = [
+    'azure-servicefabric==5.6.130',
+    'azure-cli-core',
+    'adal==0.4.3'


This is breaking CI. I have submitted #3183

msftclas added the cla-not-required label May 2, 2017

derekbekoe modified the milestone: Build Milestone May 3, 2017

derekbekoe added the P1 label May 3, 2017

troydai suggested changes May 3, 2017

View reviewed changes

troydai suggested changes May 4, 2017

View reviewed changes

johanste reviewed May 5, 2017

View reviewed changes

Adding ServiceFabric commands for version 5.6

02e97a9

samedder force-pushed the feature_servicefabric_init branch from e1ae36d to 02e97a9 Compare May 5, 2017 01:47

samedder added 2 commits May 4, 2017 19:38

Adding correct license header

1362569

Fixing Python2 compat issues and remaining style issues

29053f3

troydai approved these changes May 5, 2017

View reviewed changes

troydai merged commit 9e1433f into Azure:master May 5, 2017

yugangw-msft mentioned this pull request May 5, 2017

sf: fix an invalid adal dependency which broke CI #3183

Merged

yugangw-msft reviewed May 5, 2017

View reviewed changes

samedder deleted the feature_servicefabric_init branch May 11, 2017 20:08

haroldrandom added cla-not-required P1 labels Oct 25, 2019


		from azure.cli.core.help_files import helps

		# pylint: disable=line-too-long

		@@ -0,0 +1,154 @@
		import os

		from azure.cli.testsdk import ScenarioTest, JMESPathCheck, \

Adding support for Service Fabric client commands #3136

Adding support for Service Fabric client commands #3136

Conversation

samedder commented May 2, 2017 • edited Loading

msftclas commented May 2, 2017

samedder commented May 2, 2017

troydai commented May 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

derekbekoe commented May 3, 2017

samedder commented May 4, 2017

samedder commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troydai May 4, 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

Choose a reason for hiding this comment

samedder commented May 4, 2017 • edited Loading

troydai left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samedder commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troydai commented May 5, 2017

samedder commented May 5, 2017

troydai commented May 5, 2017 • edited Loading

samedder commented May 5, 2017

troydai commented May 5, 2017

codecov-io commented May 5, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

samedder commented May 2, 2017 •

edited

Loading

troydai May 4, 2017 •

edited

Loading

samedder commented May 4, 2017 •

edited

Loading

troydai left a comment •

edited

Loading

troydai commented May 5, 2017 •

edited

Loading

codecov-io commented May 5, 2017 •

edited

Loading