Use CLI to target other clouds#1098
Conversation
|
CONTEXT COMMAND HELP |
|
CLOUD COMMAND HELP |
|
'az account list' has also been modified. New help... |
|
For |
tjprescott
left a comment
tjprescott
left a comment
There was a problem hiding this comment.
Most comments are just discussion points, but I would like to see changes to the the CloudEndpointUrls and CloudParam classes.
| return "The cloud '{}' is already registered.".format(self.cloud_name) | ||
|
|
||
| class CannotUnregisterCloudException(Exception): | ||
| pass |
There was a problem hiding this comment.
Does this intentionally do nothing? I would recommend a comment either stating why or a TODO to indicate that future work will be expected here.
There was a problem hiding this comment.
I extend Exception so I can define my own Exception.
Like https://github.com/Azure/azure-cli/blob/master/src/azure-cli-core/azure/cli/core/_util.py#L20
| pass | ||
|
|
||
| class CloudEndpointUrl(object): # pylint: disable=too-few-public-methods | ||
| MANAGEMENT = 'management' |
There was a problem hiding this comment.
I think @BurtBiel would suggest that these be lower cased since ultimately they're just properties and not really constants. For ease of creating Azure Stack endpoint objects, I would recommend you give these objects a constructor so you can do this:
myendpoint = CloudEndpointUrl(
'managment.wonky.com',
'sql.wonky.com',
...)instead of:
myendpoint = CloudEndpointUrl()
myendpoint.MANAGEMENT = 'managment.wonky.com'
myendpoint.SQL_MANAGEMENT = 'sql.wonky.com'The all caps in the second version are even more strange because it looks like you are setting the value of a constant (which of course they aren't really constants)
Finally, the init would allow you to default certain endpoints (perhaps to public Azure?) if you wanted to.
There was a problem hiding this comment.
The class isn't used in this way.
It's more of a container for storing the key names for the cloud endpoints. Kind of like an enum with the value being a string instead of an int.
I don't call the constructor anywhere.
There was a problem hiding this comment.
But you will need to call the constructor for Stack configurations, yes? I realize it was used this way in the original implementation, which was kind of wonky, but if we are going to be populating dynamic ones, it doesn't make sense to keep that implementation.
| class CannotUnregisterCloudException(Exception): | ||
| pass | ||
|
|
||
| class CloudEndpointUrl(object): # pylint: disable=too-few-public-methods |
There was a problem hiding this comment.
Recommend changing the name to CloudEndpoints since the object itself isn't an URL, merely a container for URLs...
There was a problem hiding this comment.
Will change to CloudEndpoint.
| ACTIVE_DIRECTORY_RESOURCE_ID = 'active_directory_resource_id' | ||
| ACTIVE_DIRECTORY_GRAPH_RESOURCE_ID = 'active_directory_graph_resource_id' | ||
|
|
||
| class CloudParam(object): # pylint: disable=too-few-public-methods |
There was a problem hiding this comment.
Same general comments as above regarding __init__ and lower casing the property names. Also I would recommend naming the class CloudSuffixes since they are all URL suffixes and Param is pretty vague.
Also, the default strings here and above could just as well be empty strings because 'active_directory' isn't a valid URL suffix for anything.
There was a problem hiding this comment.
As above, the case isn't used in this way as I don't call the constructor.
Also, the default strings here and above could just as well be empty strings because 'active_directory' isn't a valid URL suffix for anything.
It's the key for the entry, not the suffix (or endpoint) itself.
Will change the name from CloudParam to CloudSuffix though.
| def __init__(self, name, endpoints=None, params=None): | ||
| self.name = name | ||
| self.endpoints = endpoints | ||
| self.params = params |
There was a problem hiding this comment.
recommend changing params to suffixes.
| if not skip_set_active_subsciption: | ||
| _set_active_subscription(get_active_context()) | ||
|
|
||
| def get_context_names(): |
There was a problem hiding this comment.
I like that "context" isn't "environment"... but context seems more vague and at least as confusing. Why not "settings"?
When I worked with Android, they use context all over the place, and I still to this day cannot really tell you what the hell a context is in the Android world (an instance of a THING is pretty close but it's very unintuitive).
There was a problem hiding this comment.
I like 'context' over 'settings' personally. Can't quite articulate why but it feels more natural than 'settings'.
I originally had 'environment' but we've had discussions about why we can't/shouldn't use that.
@johanste, @mayurid, @yugangw-msft thoughts?
There was a problem hiding this comment.
As discussed, going with 'context'. At least for now.
| # Licensed under the MIT License. See License.txt in the project root for license information. | ||
| #--------------------------------------------------------------------------------------------- | ||
|
|
||
| import os |
There was a problem hiding this comment.
Should context.py really be part of CLI core? In theory the entire CLI and the cloud selection works without any "context". If that's the case, it seems to me like it would more appropriately fit in the extensions world, since it is a user convenience.
There was a problem hiding this comment.
It needs to be part of CLI core as almost all operations require knowledge of 'the current cloud' and this is saved in context.
The user will not be exposed to context though unless they need it for their particular scenario.
| raise CLIError(e) | ||
|
|
||
| # pylint: disable=unused-argument | ||
| def register_cloud(cloud_name, |
There was a problem hiding this comment.
We should expose a "discovery_endpoint" or similar thing that if specified would attempt to query the provided URL for all of these settings. Even if not every service or cloud supports it, it would be nice for those that do.
There was a problem hiding this comment.
This does not exist yet but it's a good suggestion.
We can add support for this feature once the clouds have a discovery endpoint.
| elif arg.startswith('param_') and method_args[arg]: | ||
| cloud_to_add.params[arg.replace('param_', '')] = method_args[arg] | ||
| try: | ||
| add_cloud(cloud_to_add) |
There was a problem hiding this comment.
So if I already have a cloud's config file, how would I import that? I think this command just takes the endpoints, formats them and puts them in the user's env_config folder. It would be nice to support a JSON string here (which you could then load using @ syntax from your file) or just take a file path, validate it and move it into the necessary folder.
We could just tell people to drop that config file into the necessary directory, but that seems... clunky?
There was a problem hiding this comment.
Added support for passing in a cloud config file.
|
|
||
| MSG_GLOBAL_SETTINGS_LOCATION = 'Your global settings can be found at {}' | ||
| MSG_ACTIVE_ENV_SETTINGS_LOCATION = 'Your current environment settings can be found at {}' | ||
| MSG_ACTIVE_CONTEXT_SETTINGS_LOCATION = 'Your current context settings can be found at {}' |
There was a problem hiding this comment.
This sort of illustrates my point on "context". I have no idea what "context settings" means. However, if you took it out and it said "Your current settings can be found at..." that does make a lot more sense to me.
There was a problem hiding this comment.
Changed to just say Your current settings...
|
For CLI-specific commands (as opposed to service-related), we should always include short-descriptions for Groups (context, cloud) and commands. #Resolved |
derekbekoe
force-pushed
the
cloud-support
branch
from
3fcfdc3 to
82bc186
Compare
Squashed commits: [6b9df5b] support removing of default subscription [261310c] fix some pylint [0d312a1] Add default subscription support to context [92a97d7] name change [2361eaa] context_config to context [92dc856] az account list now has a --all option [0fa8d59] Override active context with env var [40130b6] environment -> context [14271ff] Rename environmentName to cloudName in 'account list' - 'account list' should only return accounts for the current cloud [60cf2c0] use context instead of environment [fa03638] revert todict change and commented out environment commands [a029bf9] remove todos (+9 squashed commits) Squashed commits: [53e0530] fix role and keyvault not loading [fd6e087] cloud register/unregister :) [dd00fa5] use exception convention [f214720] cloud register commands [ca6da4a] fix _CLOUD to CLOUD [e27a466] need to set base_url for SubscriptionClient [5272b9a] Remove _azure_env [fc1b6bb] new env and cloud commands [525330c] cloud list command
- vm module depends on azure-storage SDK - Storage data-plane suffix support - set base urls - After log in, we should show cloud name - fix management endpoint for german cloud - Don't remove the cached creds that match the current user when logging in - modify cloud help
Squashed commits: [126c9a8] Code review changes [cd628d1] Add support for loading registering a cloud using a json file with the params in [598fd3d] fix no dot
derekbekoe
force-pushed
the
cloud-support
branch
from
aadcaa3 to
90c851e
Compare
|
thanks! |
4 participants
Closes #651
Changes:
This change is