Skip to content

[Compute] sig image-version: add --data-snapshot-luns #12303

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.

Sign up for GitHub

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

Merged
qwordy merged 3 commits into from
Feb 27, 2020
Merged

[Compute] sig image-version: add --data-snapshot-luns #12303

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b80dd0a
[Compute] sig image-version: add --data-snapshot-luns
qwordy Feb 21, 2020
bf47e1b
Merge branch 'dev' of https://github.com/Azure/azure-cli into 12299
qwordy Feb 24, 2020
237e88e
Update help
qwordy Feb 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/azure-cli/azure/cli/command_modules/vm/_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,7 @@ def load_arguments(self, _):
c.argument('managed_image', help='image name(if in the same resource group) or resource id')
c.argument('os_snapshot', help='Name or ID of OS disk snapshot')
c.argument('data_snapshots', nargs='+', help='Names or IDs (space-delimited) of data disk snapshots')
c.argument('data_snapshot_luns', nargs='+', help='LUNs (space-delimited) of data disk snapshots')
Copy link
Member

@jiasli jiasli Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use the full name of "LUN" for readability, add more description and add the constraints that it should contain the same number of arguments as --data-snapshots.

Copy link
Member

@yungezz yungezz Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to align with data_snapshots on meaning, should it be data_snapshots_luns?

Copy link
Member Author

@qwordy qwordy Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use the full name of "LUN" for readability, add more description and add the constraints that it should contain the same number of arguments as --data-snapshots.

I will add full name of LUN in help.
There is a validation in custom function.

Copy link
Member Author

@qwordy qwordy Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to align with data_snapshots on meaning, should it be data_snapshots_luns?

I treat "data-snapshot-lun" as an entity, then I add a "s" to represent plural form. I think both names are reasonable.

c.argument('exclude_from_latest', arg_type=get_three_state_flag(), help='The flag means that if it is set to true, people deploying VMs with version omitted will not use this version.')
c.argument('version', help='image version')
c.argument('end_of_life_date', help="the end of life date, e.g. '2020-12-31'")
Expand Down
11 changes: 9 additions & 2 deletions src/azure-cli/azure/cli/command_modules/vm/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2983,7 +2983,7 @@ def create_gallery_image(cmd, resource_group_name, gallery_name, gallery_image_n
def create_image_version(cmd, resource_group_name, gallery_name, gallery_image_name, gallery_image_version,
location=None, target_regions=None, storage_account_type=None,
end_of_life_date=None, exclude_from_latest=None, replica_count=None, tags=None,
os_snapshot=None, data_snapshots=None, managed_image=None):
os_snapshot=None, data_snapshots=None, managed_image=None, data_snapshot_luns=None):
from msrestazure.tools import resource_id, is_valid_resource_id
ImageVersionPublishingProfile, GalleryArtifactSource, ManagedArtifact, ImageVersion, TargetRegion = cmd.get_models(
'GalleryImageVersionPublishingProfile', 'GalleryArtifactSource', 'ManagedArtifact', 'GalleryImageVersion',
Expand Down Expand Up @@ -3020,10 +3020,17 @@ def create_image_version(cmd, resource_group_name, gallery_name, gallery_image_n
source = GalleryArtifactVersionSource(id=managed_image)
if os_snapshot is not None:
os_disk_image = GalleryOSDiskImage(source=GalleryArtifactVersionSource(id=os_snapshot))
if data_snapshot_luns and not data_snapshots:
Copy link
Member

@yungezz yungezz Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, is there general way in CLI to force parameters combination?

Copy link
Member Author

@qwordy qwordy Feb 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have, otherwise, we don't need so many validation code. This is a common code pattern in Azure CLI.

raise CLIError('usage error: --data-snapshot-luns must be used together with --data-snapshots')
if data_snapshots:
if data_snapshot_luns and len(data_snapshots) != len(data_snapshot_luns):
raise CLIError('usage error: Length of --data-snapshots and --data-snapshot-luns should be equal.')
if not data_snapshot_luns:
data_snapshot_luns = [i for i in range(len(data_snapshots))]
data_disk_images = []
for i, s in enumerate(data_snapshots):
data_disk_images.append(GalleryDataDiskImage(source=GalleryArtifactVersionSource(id=s), lun=i))
data_disk_images.append(GalleryDataDiskImage(source=GalleryArtifactVersionSource(id=s),
Copy link
Member

@yungezz yungezz Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if data_snapshot_luns None?

Copy link
Member Author

@qwordy qwordy Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assign luns 0, 1, 2, ..., n - 1

if not data_snapshot_luns:
  data_snapshot_luns = [i for i in range(len(data_snapshots))]

lun=data_snapshot_luns[i]))
storage_profile = GalleryImageVersionStorageProfile(source=source, os_disk_image=os_disk_image,
data_disk_images=data_disk_images)
image_version = ImageVersion(publishing_profile=profile, location=location, tags=(tags or {}),
Expand Down
Loading