Updated Python SDK version and added property management commands #53
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: There was a problem hiding this comment. Ok will do, thanks. |
||
| 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'. |
||
| group.command('put', 'put_property') | ||
| group.command('list', 'get_property_info_list') | ||
| group.command('get', 'get_property_info') | ||
| group.command('delete', 'delete_property') | ||
|
|
||
| # Custom commands | ||
|
|
||
| with CommandSuperGroup(__name__, self, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is not passing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've ran the travis script locally in a virtualenv and can repeat the failure.
pip install adalwill 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a dependency of version 6.0.2 of
azure-servicefabricthen 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.