To support an input for RG name#494
Conversation
Added support for additional temporary RG location.
Updated the temporary rg name to be dynamic input.
|
|
|
If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR: |
|
@tamirkamara can you review? |
|
I'll will take care of it this week |
tamirkamara
left a comment
There was a problem hiding this comment.
Just making sure you're aware that in the scenario where the executing identity doesn't have permission to create the temp resource group, you won't be able to ask for a cleanup
| c.argument('timeout', options_list=['--timeout'], type=int, default=3600, | ||
| help='Time in seconds for the copy operation to finish. Increase this time if ' | ||
| 'you are going to copy large images (disks) like 512GB or more.') | ||
| c.argument('temporary_rg_name', options_list=['--temp-rg-name'], default='image-copy-rg', |
There was a problem hiding this comment.
please rename the option and var name without abbreviations: temporary_resource_group_name
| # pylint: disable=too-many-locals | ||
| def imagecopy(source_resource_group_name, source_object_name, target_location, | ||
| target_resource_group_name, source_type='image', cleanup='false', | ||
| temporary_rg_name, target_resource_group_name, source_type='image', cleanup='false', |
There was a problem hiding this comment.
- I think it makes sense to have both target parameters together, so please move the new temporary_rg_name parameter after target_resource_group_name
- Shouldn't the new parameter have a default value set here too, like all others have?
There was a problem hiding this comment.
Sorry, not really sure what I did with 'approve these changes.' :) thought that was approving them for my PR.
I believe it should have a default value as well as the other ones. Do I need to make these changes within my source and re-submit the PR or can it be changed within this view directly?
There was a problem hiding this comment.
You should make changes in your source code and push to you fork. The PR will be updated automatically without the need to re-submit it.
There was a problem hiding this comment.
BTW, looks like you also need to do a merge with the latest changes on the master branch
|
@ryholt @carpenike ping for updates |
merge changes to local branch
|
@ryholt I'm ok with your changes but the CI is failing and blocks my approval |
|
Yup going to try to get it fixed this morning. The lint is failing because the line is too long. |
There was a problem hiding this comment.
Thank you!
@williexu lets merge this (with squash if possible...)
Our environment does not allow users to create new RGs. Therefore, the RG needs to exist and be provided as an input for the temporary storage account location.