account: expose show command #1197
Conversation
| @@ -218,14 +218,15 @@ def get_current_account_user(self): | |||
|
|
|||
| return active_account[_USER_ENTITY][_USER_NAME] | |||
|
|
|||
| def get_subscription(self, subscription_id=None): | |||
| def get_subscription(self, subscription_id=None, subscription_name=None): | |||
There was a problem hiding this comment.
Why not simply call this subscription? The effect of calling the method and passing the command's subscription_name_or_id field into both parameters has the same effect of checking both sets of conditional logic but makes the method signature seem odd. You can simply document in the docstring for the method that subscription accepts a name or ID.
There was a problem hiding this comment.
I thought about that as well, let us merge them into one. The reason i kept them as separate was for not introduce breaking change with renaming, but we are not there yet to maintain the contract that strictly
| subscriptions = self.load_cached_subscriptions() | ||
| if not subscriptions: | ||
| raise CLIError("Please run 'az login' to setup account.") | ||
|
|
||
| result = [x for x in subscriptions if ( | ||
| subscription_id is None and x.get(_IS_DEFAULT_SUBSCRIPTION)) or | ||
| (subscription_id == x.get(_SUBSCRIPTION_ID))] | ||
| (not subscription_id and not subscription_name) and x.get(_IS_DEFAULT_SUBSCRIPTION)) or |
There was a problem hiding this comment.
likewise here you would simplify to
if (not subscription and x.get(_IS_DEFAULT_SUBSCRIPTION))
or (subscription in [x.get(_SUBSCRIPTION_ID), x.get(_SUBSCRIPTION_NAME)])) | @@ -27,6 +27,11 @@ def list_subscriptions(list_all=False): # pylint: disable=redefined-builtin | |||
| sub['cloudName'] = sub.pop('environmentName', None) | |||
| return [sub for sub in subscriptions if list_all or sub['cloudName'] == CLOUD.name] | |||
|
|
|||
| def show_subscription(subscription_name_or_id=None): | |||
There was a problem hiding this comment.
| @@ -24,4 +24,4 @@ def get_subscription_id_list(prefix, **kwargs):#pylint: disable=unused-argument | |||
| register_cli_argument('logout', 'username', help='account user, if missing, logout the current active account') | |||
|
|
|||
| register_cli_argument('account', 'subscription_name_or_id', options_list=('--name', '-n'), help='Name or ID of subscription.', completer=get_subscription_id_list) | |||
There was a problem hiding this comment.
IMO it is strange to have this be aliased to --name if it actually accepts name or ID. I would recommend just using --subscription. The account is technically separate from subscription anyways since one account can have many subscriptions (yes?) so technically --name should refer to an account. This one is kind of a gray area.
There was a problem hiding this comment.
One account == One subscription, but I have started to felt the subscription looks better. Let us go ahead
|
Also, one thing I've tried to do is when I "touch" a module I do a quick look through help to make sure there is acceptable help text for all commands and groups since we have a lot of gaps and help reflected from sloppy SDK docstrings. In this case it looks like it's just |
| @@ -168,7 +168,6 @@ def remove(self, name, handler): | |||
| @staticmethod | |||
| def _register_builtin_arguments(**kwargs): | |||
| global_group = kwargs['global_group'] | |||
| global_group.add_argument('--subscription', dest='_subscription_id', help=argparse.SUPPRESS) | |||
There was a problem hiding this comment.
@tjprescott, I will wire it up differently when address #833. For now as we both know this flag is doing nothing
EDIT, the issue is #807
fix #1101