image-copy: first commit - #8
Conversation
|
|
||
| def load_params(_): | ||
| with ParametersContext('image copy') as c: | ||
| c.register('source_resource_group_name', '--source-resource-group-name', help='Name of the resource gorup of the source resource') |
There was a problem hiding this comment.
I would just make it --source-resource-group or maybe --source-group. --source-resource-group-name is too long.
|
|
||
| def load_params(_): | ||
| with ParametersContext('image copy') as c: | ||
| c.register('source_resource_group_name', '--source-resource-group-name', help='Name of the resource gorup of the source resource') |
There was a problem hiding this comment.
Typo gorup to group
| with ParametersContext('image copy') as c: | ||
| c.register('source_resource_group_name', '--source-resource-group-name', help='Name of the resource gorup of the source resource') | ||
| c.register('source_object_name', '--source-object-name', help='The name of the image or vm resource') | ||
| c.register('target_location', '--target-location', help='Comma seperated location list to create the image in') |
There was a problem hiding this comment.
Typo seperated
There was a problem hiding this comment.
This should use nargs=+ and it would be space-separated since that's what we use elsewhere in CLI.
For example https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/commands/parameters.py#L209
| c.register('source_resource_group_name', '--source-resource-group-name', help='Name of the resource gorup of the source resource') | ||
| c.register('source_object_name', '--source-object-name', help='The name of the image or vm resource') | ||
| c.register('target_location', '--target-location', help='Comma seperated location list to create the image in') | ||
| c.register('source_type', '--source-type', help='image (default) or vm') |
There was a problem hiding this comment.
What's the (default) for?
Not sure if I understand the help text fully and maybe it's not that clear?
| c.register('source_object_name', '--source-object-name', help='The name of the image or vm resource') | ||
| c.register('target_location', '--target-location', help='Comma seperated location list to create the image in') | ||
| c.register('source_type', '--source-type', help='image (default) or vm') | ||
| c.register('target_resource_group_name', '--target-resource-group-name', help='Name of the resource group to create images in') |
There was a problem hiding this comment.
I would remove name from '--target-resource-group-name at least.
|
|
||
| source_os_disk_id = json_cmd_output['storageProfile']['osDisk']['managedDisk']['id'] | ||
| source_os_type = json_cmd_output['storageProfile']['osDisk']['osType'] | ||
| logger.debug("source_os_disk_id: " + source_os_disk_id + " source_os_type: " + source_os_type) |
There was a problem hiding this comment.
Logger prefers lazy loading of arguments so:
logger.debug("source_os_disk_id: %s source_os_type: %s", source_os_disk_id, source_os_type)
| pool.close() | ||
| pool.join() | ||
| except KeyboardInterrupt: | ||
| print('xxx - parent') |
| except KeyboardInterrupt: | ||
| print('xxx - parent') | ||
| logger.warn('User cancelled the operation') | ||
| if 'true' in cleanup: |
There was a problem hiding this comment.
If cleanup is defined as action='store_true' then this would simply be if cleanup with is a lot less likely to break e.g. with letter casing.
| print('xxx - parent') | ||
| logger.warn('User cancelled the operation') | ||
| if 'true' in cleanup: | ||
| logger.warn('To cleanup temporary resources look for ones tagged with "image-copy-extension"') |
There was a problem hiding this comment.
Would be good if you actually print the command to do this.
az resource list --tag ...
| license='MIT', | ||
| author='Tamir Kamara', | ||
| author_email='tamir.kamara@microsoft.com', | ||
| url='https://github.com/ORG/REPO', |
There was a problem hiding this comment.
|
Tamir Kamara (@tamirkamara) Thanks for the PR! |
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "azext.minCliVersion": "2.0.12" | |||
| } No newline at end of file | |||
There was a problem hiding this comment.
You can remove this.
I know it was in the example but not needed in this case.
| c.register('source_resource_group_name', '--source-resource-group', help='Name of the resource group of the source resource') | ||
| c.register('source_object_name', '--source-object-name', help='The name of the image or vm resource') | ||
| c.register('target_location', '--target-location', nargs='+', help='Space separated location list to create the image in (use location short codes like westeurope etc.)') | ||
| c.register('source_type', '--source-type', default='image', help='image or vm') |
There was a problem hiding this comment.
Add choices=['image', 'vm'] if those are the only two allowed values.
| json_output = json.loads(cmd_output) | ||
| return json_output | ||
| else: | ||
| raise CLIError("Command returned an unexpected empty string.") |
There was a problem hiding this comment.
Would be good to add which command to this error message?
There was a problem hiding this comment.
There's one later on - this raises an exception in the try-catch block and the catch prints the cmd
|
|
||
| # tag newly created resources | ||
| if 'create' in cmd and ('container' not in cmd): | ||
| full_cmd += ['--tags', 'created_by=image-copy-extension'] |
There was a problem hiding this comment.
Okay makes sense.
Consider adding one line comment for this to explain?
|
|
||
| if 'false' in cmd_output: | ||
| # create the target resource group | ||
| logger.warn("Creating resource group: %s", ) |
There was a problem hiding this comment.
Missing the actual value here it seems
| url='https://github.com/Azure/azure-cli-extensions', | ||
| classifiers=CLASSIFIERS, | ||
| packages=find_packages(), | ||
| package_data={'azext_imagecopy': ['azext_metadata.json']}, |
There was a problem hiding this comment.
You can remove this.
2 participants
Derek Bekoe (@derekbekoe) , coscor-ms please review