New-AzureRmVm/Vmss: Custom image #5721
Conversation
sergey-shandar
changed the title
| || providerNamespace != ComputeStrategy.Namespace | ||
| || provider != "images") | ||
| { | ||
| throw new ArgumentException("Invalid image resource id '" + imageName + "'."); |
There was a problem hiding this comment.
This should be in a resource string
| var compute = client.GetClient<ComputeManagementClient>(); | ||
| if (compute.SubscriptionId != subscriptionId) | ||
| { | ||
| throw new ArgumentException("The image subscription doesn't math the current subscription."); |
There was a problem hiding this comment.
resource string, also 'math' -> 'match'
| { | ||
| // get image | ||
| return Images | ||
| var compute = client.GetClient<ComputeManagementClient>(); |
There was a problem hiding this comment.
We shouldn't duplicate this code -it feels like this should be in the same if case as if the image contains a '/'
There was a problem hiding this comment.
I would suggest first checking if the name contains a ':', then a single case for '/' or no '/'.
| { | ||
| throw new ArgumentException("Invalid image resource id '" + imageName + "'."); | ||
| } | ||
| // has to be "" |
There was a problem hiding this comment.
I would move image resource id parsing into a helper method, or better yet, use this common code, and simply ensure that the properties are what you expect: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Common/Commands.ResourceManager.Common/Utilities/Models/ResourceIdentifier.cs
This will give you subscription, resourceGroupName, resourceName and resourceType (Microsoft.Comute/images)
| if (result == null) | ||
| { | ||
| // TODO: move it to resource. | ||
| throw new ArgumentException("Can't find the image '" + imageName + "'"); |
sergey-shandar
changed the title
| /// </summary> | ||
| /// <param name="id"></param> | ||
| /// <returns></returns> | ||
| public static IResourceId TryParse(string id) |
There was a problem hiding this comment.
@sergey-shandar any reason you implemented this class instead of using the existing ResourceIdentifier class?
There was a problem hiding this comment.
We also need this interface in template implementation. The ARM template implementation is inside a Strategy library which doesn't depend on a library which implements ResourceIdentifier class.
| return Images | ||
| try | ||
| { | ||
| var localImage = await compute.Images.GetAsync(resourceGroupName, imageName); |
There was a problem hiding this comment.
@sergey-shandar does this address Mark's comments?
We shouldn't duplicate this code -it feels like this should be in the same if case as if the image contains a '/'. I would suggest first checking if the name contains a ':', then a single case for '/' or no '/'.
From the comment, it seems like the above else if should just be an else.
There was a problem hiding this comment.
The implementations are quite different and we don't want invalid image ids to pass as image names.
4 participants
Description
New-AzureRmVm: Custom image.
Currently, only an image in the same resource group is supported. Id is not supported yet.
Issue #5660
Depends on PR #5722
Checklist
CONTRIBUTING.mdplatyPSmodule