Adding provision application type, get container logs deployed on nod… #64

Christina-Kang · 2018-02-05T19:45:58Z

add new version of provision application type
add command to get container logs deployed on node
make queries with max results option accept max results option
add automated testing to verify correctness of generated HTTP request
added myself to repo owners
changed verify.sh script to not close automatically after run on windows (not tested on linux)
updated change log
updated service fabric SDK required version
changed command sfctl property put to be more flat with its parameters
fixed bug with infrastructure commands erroring out
Change imagestore upload command to generate rest API with version 6.1 instead of 3.0-preview

…e, and testing

samedder · 2018-02-08T19:40:28Z

CODEOWNERS

 # review when someone opens a pull request.
-*       @jeffreychenmsft @samedder
+*       @jeffreychenmsft @samedder @christina-kang


please remove @jeffreychenmsft

samedder · 2018-02-08T19:41:09Z

scripts/verify.sh

 fi
+


If you want to add a windows command please add these to a separate file, something like verify.ps1.

I don't have access to a Linux machine - out of curiosity, does the window stay open on linux?

I've added a file called verifyWindows.sh with these changes, and left verify.sh as is. I'll add docs to the wiki page if this change goes through

Why .sh? You can just call it verify.bat or verify.ps1 depending on why you write it for PS or CMD.

Because I don't want to have to change other parts of the the script. It doesn't seem worth the effort.

samedder · 2018-02-08T19:41:25Z

src/README.rst

+3.2.0
+-----
+
+- Update to 6.1 Service Fabric runtime


Merge from master, new version is 4.0.0

samedder · 2018-02-08T19:41:44Z

src/setup.py

@@ -46,7 +46,7 @@ def read(fname):
        'knack==0.1.1',
        'msrest>=0.4.4',
        'requests',
-        'azure-servicefabric==6.0.2',
+        'azure-servicefabric>=6.1.1',


Same as before, merge from master, take master's changes

samedder · 2018-02-08T19:43:04Z

src/sfctl/custom_app.py

@@ -12,6 +12,112 @@
 import sys
 import shutil
 from knack.util import CLIError
+from msrest.pipeline import ClientRawResponse


I didn't see you added duplicate work here but I added some of this logic in a new file called custom_app_type, try merging there please. Sorry for making you do extra work ☹️

I'll leave this for now, since I plan on moving a bunch of commands around. Right now, provision is under application, where it has been for a while.

Once the SDK is fixed, then we can switch to your command :) Turns out it's not quite duplicated because mine completely is not dependent on the SDK, because at the time of writing there was an issue there, but it has since been fixed.

So chose to keep one implementation, either yours or mine, I don't particularly care which one.

Also please move it to a separate file like I did since we will hit the file line length pylint warning if we keep adding everything to a single file. It's a readability issue.

Let's keep this one for now, and I'll shorten the file length as part of the next change, which will move this out of the app subgroup.

Can you just do it now? :)

Can you just move it to a separate file now?

Moved to custom_app_type :) I've renamed your command for now so it doesn't conflict.

You can remove the file entirely, it's in Git, so there's no reason to have dead code lying around

samedder · 2018-02-08T19:44:29Z

src/sfctl/custom_is.py

+    """Invokes an administrative command on the given Infrastructure Service
+    instance.
+
+    For clusters that have one or more instances of the Infrastructure


Don't add long docstings in the custom files, please add using the YAML format for help files under the helps folder.

I think you mentioned it before, but I can't remember; why do we avoid putting it here? This is the same method that we get the docs if we were using the SDK, so this is much easier to maintain, since we can copy and paste things over directly, in case there are changes that need to be made to it.

For example, I find myself having to manually redo some corrections I have made in swagger in the help files. If the specific case does not require formatting which cannot be produced outside of the help files, then it is much easier to bulk replace everything in the custom command file.

Readability for the function definitions

YAML help strings features are a superset of docstring features

Consistency with other Knack-based commands

moved to help file.

samedder · 2018-02-08T19:48:55Z

src/sfctl/tests/scenario_test.py

@@ -12,7 +12,10 @@
 from sfctl.entry import cli
 from sfctl.tests.helpers import (ENDPOINT, MOCK_CONFIG)

-
+# VCR recording is used to aid with these tests.


samedder · 2018-02-08T19:49:38Z

src/sfctl/tests/scenario_test.py

-            'applicationHealthStates[0].name',
-            'fabric:/System'
-        )])
+        self.cmd('cluster health')


Does cluster health not return these properties any longer?

I can't remember why I removed it, but I will remove this test all together. Pls see replacement in request_generation_test:

self.validate_command( # health 'sfctl cluster health --applications-health-state-filter=2 \ --events-health-state-filter=2 \ --include-system-application-health-statistics \ --nodes-health-state-filter=2', 'GET', '/$/GetClusterHealth', ['api-version=6.0', 'NodesHealthStateFilter=2', 'ApplicationsHealthStateFilter=2', 'EventsHealthStateFilter=2', 'IncludeSystemApplicationHealthStatistics=true'])

Leaving the other tests for now

…into bikangall

coveralls · 2018-02-09T21:35:46Z

Coverage increased (+21.9%) to 71.817% when pulling a7066c3 on Christina-Kang:bikangall into 32b4a25 on Azure:master.

samedder

Make sure CI passes also

samedder · 2018-02-09T22:55:02Z

scripts/verifyWindows.sh

@@ -0,0 +1,33 @@
+#!/bin/bash


Is this meant to target WSL? Python works on windows without WSL also, should just make a verify.bat file that runs in CMD or even better verify.ps1 that uses PS.

This isn't meant to do anything different from verify.sh. The only change was adding "$SHELL" so that the pop up window doesn't close when I run this script. I can remove it if you think it's better.

I don't want to put in the time right now into adding a script for windows when this works fine.

So I think I'm confused what your intention is here. So are you trying to create a script that you can use on windows that will run pylint and nose?

samedder · 2018-02-09T22:55:37Z

src/README.rst

@@ -28,6 +28,9 @@ Change Log
 - Deployed application info can now optionally include health states
 - Numerous documentation improvements and corrections
 - ChaosContext (context) and ChaosTargetFilter (chaos-target-filter) arguments are added to Chaos start command (#62)
+- Add test structure to verify correct HTTP request generation
+- Update provision application type command to match the latest Service Fabric runtime.


I'd add a note here about moving it to a custom command

samedder · 2018-02-09T22:55:48Z

src/README.rst

@@ -28,6 +28,9 @@ Change Log
 - Deployed application info can now optionally include health states
 - Numerous documentation improvements and corrections
 - ChaosContext (context) and ChaosTargetFilter (chaos-target-filter) arguments are added to Chaos start command (#62)
+- Add test structure to verify correct HTTP request generation
+- Update provision application type command to match the latest Service Fabric runtime.
+- Add command to get container logs deployed on node.


samedder · 2018-02-09T22:57:07Z

src/sfctl/custom_app.py

@@ -12,6 +12,112 @@
 import sys
 import shutil
 from knack.util import CLIError
+from msrest.pipeline import ClientRawResponse


Can you just move it to a separate file now?

samedder · 2018-02-09T22:57:29Z

src/sfctl/custom_app.py

+
+    from azure.servicefabric.models.provision_application_type_description \
+       import (ProvisionApplicationTypeDescription)
+    from azure.servicefabric.models.external_store_provision_application_type_description import (ExternalStoreProvisionApplicationTypeDescription) # pylint: disable=line-too-long


Use \ here, I think the max line length should be 100 now.

samedder · 2018-02-09T23:02:25Z

src/sfctl/custom_app.py

+                application_type_name=application_type_name,
+                application_type_version=application_type_version)
+
+    api_version = "6.1"


I believe this is still the broken api_version 😛

I need to add the changes to serialize the body ourselves (if feasible). Otherwise, we will not have new functionality.

Sounds good, up to you how you want to implement it.

samedder · 2018-02-09T23:02:58Z

src/sfctl/custom_app.py

+    query_parameters = {}
+    query_parameters['api-version'] = client._serialize.query( # pylint: disable=W0212
+        "api_version", api_version, 'str')
+    if timeout is not None:


This will never be false.

samedder · 2018-02-09T23:03:37Z

src/sfctl/custom_app.py

+       import (ProvisionApplicationTypeDescription)
+    from azure.servicefabric.models.external_store_provision_application_type_description import (ExternalStoreProvisionApplicationTypeDescription) # pylint: disable=line-too-long
+
+    from azure.servicefabric import models


Import only the models you use.

Changed to from azure.servicefabric.models import FabricErrorException

samedder · 2018-02-09T23:05:56Z

src/sfctl/helps/app.py

+          short-summary: The location from where application package can be registered or provisioned. Indicates that the provision is for a package that was previously uploaded to the image store.
+        - name: --external-store-provision
+          type: string
+          short-summary: The location from where application package can be registered or provisioned. Indicates that the provision is for an application package that was previously uploaded to an external store. The application package ends with the extension *.sfpkg.


Check line length?

samedder · 2018-02-09T23:07:34Z

src/sfctl/tests/mock_server.py

+class MockServer(BaseHTTPRequestHandler):
+    """ Overrides the following methods in BaseHTTPRequestHandler """
+
+     # pylint: disable=E1101


Don't use the number format identifiers, use the strings like invalid-name or line-too-long

samedder · 2018-02-14T22:30:04Z

@Christina-Kang rather than replying 'fixed' or 'updated', just push your changes when you've addressed the comments and I'll see they get removed. You only really need to respond to comments if you disagree with the feedback.

samedder · 2018-02-15T01:36:51Z

scripts/verifyWindows.sh

@@ -0,0 +1,33 @@
+#!/bin/bash


So I think I'm confused what your intention is here. So are you trying to create a script that you can use on windows that will run pylint and nose?

samedder · 2018-02-15T01:38:17Z

src/sfctl/commands.py

+                               client_factory=client_create) as super_group:
+            with super_group.group('property') as group:
+                group.command('put', 'naming_property_put')
+
        # Only add when provision API correctly specified in SDK


Does this line need to be removed now?

samedder · 2018-02-15T01:39:34Z

src/sfctl/custom_app_type.py

+            raise CLIError('Missing required parameter '
+                           '--application-type-build-path.')
+
+        provision_description = \


No need to use \ here

samedder · 2018-02-15T01:41:14Z

src/sfctl/custom_app_type.py

-            no_wait,
-            application_type_build_path=application_type_build_path
-        )
+        if not all([application_package_download_uri, application_type_name, \


Might want to add a check that someone doesn't also specify application_type_build_path

samedder · 2018-02-15T01:41:39Z

src/sfctl/custom_app_type.py

+            raise CLIError('Missing required parameters. The following are required: '
+                           '--application-package-download-uri, --application-type-name, '
+                           '--application-type-version.')
+        provision_description = \


Again use ( to avoid \

samedder · 2018-02-15T21:30:32Z

src/sfctl/custom_app_type.py

+            body_content_sorted[key] = body_content[key]
+
+    if list(body_content_sorted.keys())[0] != "Kind":
+        raise CLIError('Internal CLI error: Kind must be the first item to be serialized.')


If this an internal error, then it shouldn't be a CLIError, it should be a different kind. The type CLIError is meant to be user-actionable, like a wrong parameter or something else the user can immediately fix.

samedder · 2018-02-15T21:31:08Z

src/sfctl/custom_service.py

@@ -162,7 +162,8 @@ def validate_service_create_params(stateful, stateless, singleton_scheme, #pylin
            'Specify either stateful or stateless for the service type'
        )
    if sum([singleton_scheme, named_scheme, int_scheme]) != 1:
-        raise CLIError('Specify exactly one partition scheme')
+        raise CLIError('Specify exactly one partition scheme from --singleton_scheme, '


Arguments use - not _

samedder · 2018-02-15T21:31:55Z

src/sfctl/helps/property.py

+# license information.
+# -----------------------------------------------------------------------------
+
+#pylint: disable=line-too-long


Don't disable line too long here

samedder

Only minor .gitignore, otherwise looks good.

samedder · 2018-02-20T18:39:21Z

.gitignore

@@ -3,6 +3,9 @@ __pycache__/
 *.py[cod]
 *$py.class

+# Scripts


Don't modify the .gitignore here

Adding provision application type, get container logs deployed on nod…

523a473

…e, and testing

Christina-Kang requested review from jeffreychenmsft and samedder as code owners February 5, 2018 19:45

samedder requested changes Feb 8, 2018

View reviewed changes

samedder self-assigned this Feb 8, 2018

samedder added this to the 6.1 SF Runtime Release milestone Feb 8, 2018

Christina-Kang added 2 commits February 8, 2018 15:47

Merge branch 'master' of https://github.com/Azure/service-fabric-cli …

ed23c78

…into bikangall

Changes based on CR

a29b46b

Christina-Kang added 4 commits February 9, 2018 13:47

fix infrastructure help

b3981bd

return from mock server tests if python version < 3

08af5ec

fixes for python 2.7 compat

14dbc86

backwards compat

bbd4d61

samedder requested changes Feb 9, 2018

View reviewed changes

Christina-Kang added 2 commits February 9, 2018 15:12

forgot import statement

56257fa

updated according to comments

599027a

Christina-Kang added 2 commits February 14, 2018 15:15

testing doc fixes

3446e7f

fixed python 2.7 compat error

deb32cc

samedder requested changes Feb 15, 2018

View reviewed changes

Christina-Kang added 4 commits February 15, 2018 14:16

remove verifyWindows.sh from git only

1afa40b

fixed based on PR comments

ef3986c

changed msrest version to force latest

7ab93c3

fixed comment i forgot to fix in last push

2539ee6

samedder requested changes Feb 20, 2018

View reviewed changes

.gitignore Outdated

@@ -3,6 +3,9 @@ __pycache__/

*.py[cod]

*$py.class

# Scripts

Copy link

Collaborator

samedder Feb 20, 2018

Choose a reason for hiding this comment

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

Don't modify the .gitignore here

Removing verifyWindows from gitignore

a7066c3

samedder approved these changes Feb 20, 2018

View reviewed changes

samedder merged commit b2666bb into microsoft:master Feb 20, 2018

samedder mentioned this pull request Feb 22, 2018

Provision command removed due to breaking API #67

Closed

vturecek unassigned samedder May 23, 2018

Christina-Kang deleted the bikangall branch November 1, 2018 21:23

Adding provision application type, get container logs deployed on nod… #64

Adding provision application type, get container logs deployed on nod… #64

Conversation

Christina-Kang commented Feb 5, 2018

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

Christina-Kang Feb 9, 2018 • 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

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

Christina-Kang Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

coveralls commented Feb 9, 2018 • edited Loading

samedder 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

samedder commented Feb 14, 2018

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Christina-Kang Feb 9, 2018 •

edited

Loading

Christina-Kang Feb 9, 2018 •

edited

Loading

coveralls commented Feb 9, 2018 •

edited

Loading