Updated Python SDK version and added property management commands #53
Conversation
jjcollinge
changed the title
| @@ -46,7 +46,7 @@ def read(fname): | |||
| 'knack==0.1.1', | |||
| 'msrest>=0.4.4', | |||
| 'requests', | |||
| 'azure-servicefabric==6.0', | |||
| 'azure-servicefabric==6.0.2', | |||
There was a problem hiding this comment.
I've ran the travis script locally in a virtualenv and can repeat the failure. pip install adal will resolve the issue - do you want me to add 'adal' to the requirements.txt or should we fix 6.0.2 dependency graph to ensure the package is pulled in?
There was a problem hiding this comment.
If it's a dependency of version 6.0.2 of azure-servicefabric then the requirements for the package must be updated. I don't think your changes here require adal explicitly.
There was a problem hiding this comment.
It seems that between version 6.0.0 and 6.0.1 of azure-servicefabric that the msrestazure module is not being pulled in (although it still remains referenced in the requirements.txt) and therefore adal (which is a dependency of msrestazure) is not pulled in either. There is an explicit reference to 'adal' in src/sfctl/auth.py therefore I think it makes sense to put it in sfctl's requirements.txt rather than rely on an external package's dependency - I'm no expert in python dependencies so happy to take your advice on this.
There was a problem hiding this comment.
Yeah it's fine to add it as a dependency for sfctl. I must have missed that whoever authored the auth changes didn't add that package.
| @@ -203,6 +203,12 @@ def load_command_table(self, args): #pylint: disable=too-many-statements | |||
| group.command('command', 'invoke_infrastructure_command') | |||
There was a problem hiding this comment.
Add help text for the property group now
There was a problem hiding this comment.
Is an additional help file required for a standard command group?
There was a problem hiding this comment.
You added a group of commands: sfctl property. In the same way there's documentation on sfctl application. Take a look at how the help is defined for that group.
There was a problem hiding this comment.
Ok will do, thanks.
| @@ -203,6 +203,12 @@ def load_command_table(self, args): #pylint: disable=too-many-statements | |||
| group.command('command', 'invoke_infrastructure_command') | |||
| group.command('query', 'invoke_infrastructure_query') | |||
|
|
|||
| with super_group.group('property') as group: | |||
There was a problem hiding this comment.
You should add scenario tests for this, take a look at the existing tests to see how to write scenario tests and the wiki documentation
There was a problem hiding this comment.
I had a look at writing some scenario tests - the only Service Fabric name that I can guarantee is in the cluster is the 'System' name. I can test the 'list' and 'get' against it but it's restricted as far as 'put' and 'delete'.
I can either deploy an application using the 'sfctl application' commands or update this PR to include a new property group for 'name' (i.e. sfctl name create "{"Name":"TestName"}") and couple the property tests to the name commands. How do you suggest I proceed? Docs on this API are here: https://docs.microsoft.com/en-us/rest/api/servicefabric/sfclient-index-property-management
|
I forgot to mention in the review but please also update
|
src/setup.py
Outdated
| @@ -17,7 +17,7 @@ def read(fname): | |||
|
|
|||
| setup( | |||
| name='sfctl', | |||
| version='3.0.0', | |||
| version='3.0.1', | |||
There was a problem hiding this comment.
I'd say lets make this 3.1.0
|
@jjcollinge Looks good except for just the version. Do you have any tests/verification for this btw? I'm a bit weary checking this in without at least some minimal verification that the empty cases work correctly. |
|
I updated the version. As it's a standard command there's no logic to unit test as such but I've added a few scenario tests. The only one I can positively validate on a 'clean' SF cluster is 'sfctl property list --name "System"' as it's the only name that exists. All other operations against "System" are restricted. The response for error conditions (i.e. referencing a non existent property or name) will just spit back the raw exception/stack trace with an error message such as (FabricErrorException: (FABRIC_E_PROPERTY_DOES_NOT_EXIST) Property does not exist). I would add logic to prettify this for the user but that's inconsistent with the rest of the sfctl commands. |
|
Thanks for the contribution @jjcollinge |
This PR updates the azure-servicefabric Python SDK to v6.0.2 and exposes a subset of the Property Management APIs via standard commands.
I've only added commands relevant to modifying properties for existing Service Fabric names i.e. 'GettingStartedApplication/WebService' rather than the full name/property API surface. This works for our use case (adding app/service metadata) but appreciate you may want the commands to map to a more generalised API surface.
Example usage can be seen below: