[ACS] Add support for top-level browse and install-cli commands.#1476
[ACS] Add support for top-level browse and install-cli commands.#1476tjprescott merged 2 commits intoAzure:masterfrom
Conversation
|
@brendandburns, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sanar-microsoft, @derekbekoe and @colemickens to be potential reviewers. |
derekbekoe
changed the title
brendandburns
force-pushed
the
connect
branch
from
d1637a6 to
d38e9b8
Compare
| orchestrator_type = acs_info.orchestrator_profile.orchestrator_type # pylint: disable=no-member | ||
| if orchestrator_type == 'kubernetes': | ||
| # TODO: is there a more python idiomatic way to do this? | ||
| if client_version: |
There was a problem hiding this comment.
Aren't you guaranteed to have a value here by nature of what you have in _params.py?
There was a problem hiding this comment.
Same for the check in the dcos branch.
There was a problem hiding this comment.
Not if you come in the acs install-cli ... path, since we can't have a default in there, since it's orchestrator dependent, and we don't know which it is 'til we do the lookup.
There was a problem hiding this comment.
Now I'm curious, do you only need to register cli arguments in _params.py if you want to specify short flags or defaults?
There was a problem hiding this comment.
yes, (or other fanciness like tab-completion) I believe it otherwise picks them up by reflecting on the method (and looking at it's docstring)
There was a problem hiding this comment.
@colemickens if you want to specify defaults, the preferred way is to simply use the Python argument defaulting mechanism in the custom command signature. Using the "default" kwarg in the _params.py is discouraged.
If you don't register something in _params.py the CLI will use reflection to get the following info:
- requiredness (based on presence of a default)
- default value
- long option name
- parameter help (from a docstring).
There was a problem hiding this comment.
Hm, I had been encouraged to move the default into _params.py in [this PR] (#1448 (review)) but maybe that was because the default value wasn't a simple constant?
There was a problem hiding this comment.
@tjprescott "Using the "default" kwarg in the _params.py is discouraged."
Computed defaults should be added to _params.py though right due to Python's parameter defaulting logic - http://docs.python-guide.org/en/latest/writing/gotchas/#what-does-happen.
There was a problem hiding this comment.
Looking at what @colemickens had in PR #1448, I would agree that would be a reasonable use of default in _params.py.
|
@derekbekoe I think this is ready to go in... Thanks! |
| if client_version: | ||
| return dcos_install_cli(client_version=client_version, install_location=install_location) | ||
| else: | ||
| return dcos_install_cli(install_location=install_location) |
There was a problem hiding this comment.
You could say:
return dcos_install_cli(client_version=client_version, install_location=install_location) if client_version else dcos_install_cli(install_location=install_location)But, even more concise, you should be able to do:
kwargs = {'install_location': install_location}
if client_version:
kwargs['client_version'] = client_version
return dcos_install_cli(**kwargs)
| """ | ||
| Opens a browser to the web interface for the cluster orchestrator | ||
|
|
||
| :param name: name: Name of the target Azure container service instance. |
| orchestrator_type = acs_info.orchestrator_profile.orchestrator_type # pylint: disable=no-member | ||
| if orchestrator_type == 'kubernetes': | ||
| # TODO: is there a more python idiomatic way to do this? | ||
| if client_version: |
There was a problem hiding this comment.
@tjprescott "Using the "default" kwarg in the _params.py is discouraged."
Computed defaults should be added to _params.py though right due to Python's parameter defaulting logic - http://docs.python-guide.org/en/latest/writing/gotchas/#what-does-happen.
|
@derekbekoe we tend not to have computed defaults in the _params.py file, especially randomized ones. What we do in those cases is make the default (in the signature) However, in the case of dynamic, but essentially static defaults (like user directory paths) then using default in _params.py seems reasonable. The other time it makes sense is when you're reflecting on the SDK and you want to override its default value. |
|
Ok, comments addressed, please take another look. |
7 participants
Fixes #1358