-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Knack - Storage Command Module #5076
Conversation
View a preview at https://prompt.ws/r/Azure/azure-cli/5076 |
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.
General points:
- Accessing _cmd through the namespace in a validator should not be done. Add cmd to the validator signature and access that way.
- Where you have
cmd.supported_api_version(..)
, you should be able to omit the ResourceType, since it is presumably already set (either at the module level or command group level). - Where you are using the stand-alone
get_sdk
to retrieve models, you should be able to usecmd.get_models(...)
. That is the recommended way, so if it doesn't work for some reason, I'd be very interested to understand why as it might mean there's a bug I need to address. - I would be surprised if the completers worked as they don't conform to the signature that @completer calls for.
I pointed out a few other places where you did things that I wouldn't expect you to have to do. If the simplifications I suggest don't work, please let me know as it means there's probably an edge-case that the core doesn't handle the way it should.
c.argument('tags', tags_type) | ||
c.argument('custom_domain', help='User domain assigned to the storage account. Name is the CNAME source.') | ||
c.argument('sku', help='The storage account SKU.', arg_type=get_enum_type(SkuName, default='standard_ragrs')) | ||
c.argument('sku', help='The storage account SKU.', arg_type=get_enum_type(t_sku_name, default='standard_ragrs')) | ||
|
||
with self.argument_context('storage account update') as c: | ||
register_common_storage_account_options(c) |
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.
This could be moved into the argument context to be more idiomatic with the other statements (argument, ignore, etc).
with self.argument_context('storage blob lease') as c: | ||
c.argument('lease_duration', type=int) | ||
c.argument('lease_break_period', type=int) | ||
c.argument('blob_name', arg |
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.
These can be combined: c.ignore('blob_name', 'snapshot')
with self.argument_context('storage blob lease') as c: | ||
c.argument('lease_duration', type=int) | ||
c.argument('lease_break_period', type=int) | ||
c.argument('blob_name', arg |
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.
Combine
with self.argument_context('storage blob lease') as c: | ||
c.argument('lease_duration', type=int) | ||
c.argument('lease_break_period', type=int) | ||
c.argument('blob_name', arg |
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 you put arg_group='Copy Source'
into line 560, this should also work. less repetition. If it doesn't, it would be a bug.
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.
Works.
n = namespace | ||
|
||
def get_config_value(section, key, default): | ||
return namespace._cmd.cli_ctx.config.get(section, key, default) |
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.
This approach relies on an implementation-detail that may change. Instead of putting namespace._cmd
here, you should simply put cmd
into validate_client_parameters
signature. The you just return cmd.cli_ctx....
|
||
def _get_service_container_type(cmd, client): | ||
from ..sdkutil import get_table_data_type | ||
t_block_blob_svc, t_file_svc, t_queue_svc = get_sdk( |
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.
Why does cmd.get_models
not work here?
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.
Because for storage multiapi package the models are in different namespaces. For example, blob.modes
and file.models
.
@@ -174,13 +180,13 @@ def upload_append_blob(): | |||
'container_name': container_name, | |||
'blob_name': blob_name, | |||
'file_path': file_path, | |||
'progress_callback': _update_progress, | |||
'progress_callback': get_update_progress_fn(cmd), |
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.
Does this show progress for each file? If so, it seems very much like with Tamir is trying to do in the image copy extension. If this works for us, it might work in his situation.
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.
yes, it does.
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.
Please touch base with him and see if this helps with his image_copy extension, since I'm assuming this doesn't break due to the cli_ctx.
'maxsize_condition': maxsize_condition, | ||
'lease_id': lease_id, | ||
'timeout': timeout | ||
} | ||
|
||
if supported_api_version(ResourceType.DATA_STORAGE, min_api='2016-05-31'): | ||
if cmd.supported_api_version(ResourceType.DATA_STORAGE, min_api='2016-05-31'): |
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 wouldn't expect that you would need to specify the ResourceType.
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.
will update.
|
||
|
||
def list_share_directories(cmd, client, share_name, directory_name=None, timeout=None): | ||
t_dir_properties = get_sdk(cmd.cli_ctx, ResourceType.DATA_STORAGE, 'file.models#DirectoryProperties') |
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.
Again, here you should just be able to call cmd.get_models('file.models#DirectoryProperties')
|
||
|
||
@Completer | ||
def file_path_completer(cmd, prefix, action, parsed_args, **kwargs): # pylint: disable=unused-argument |
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.
Do these completers work? The signature is supposed to be def my_completer(cmd, prefix, namespace, [**kwargs])
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.
What should be changed to the completer to make it conform the expected signature?
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.
Remove action
and rename parsed_args
to namespace
.
kwargs.pop('account_key', None), | ||
connection_string=kwargs.pop('connection_string', None), | ||
sas_token=kwargs.pop('sas_token', None)) | ||
t_file_svc = get_sdk(cli_ctx, ResourceType.DATA_STORAGE, 'file#FileService') |
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.
Why use t_file_svc
instead of the original name of FileService
?
Same everywhere else.
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.
In my opinion this is a better name convention. The original one is misleading. The reader is not able to tell if it is a type actually defined in source code or a type from the SDK. Alternatively, we can adopt this pattern: https://www.python.org/dev/peps/pep-0008/#type-variable-names (of course it makes your code very "C" like).
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.
ok fair
.flake8
Outdated
@@ -14,7 +14,7 @@ exclude = | |||
scripts | |||
doc | |||
build_scripts | |||
src/command_modules/azure-cli-storage | |||
src/command_modules/azure-cli-lab |
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.
This looks like a bad rebase. Lab should be included in flake 8 (and thus shouldn't be in this list).
Fixes: #4941
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Command Guidelines
(see Authoring Command Modules)