Remove Template Capture from ARM Builder And Fail Fast when SIG Version already exists #3
Conversation
…eplicated to DTL/Chroot
| @@ -206,6 +203,14 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("the Shared Gallery Image '%s' to which to publish the managed image version to does not exist in the resource group '%s' or does not contain managed image '%s'", b.config.SharedGalleryDestination.SigDestinationGalleryName, b.config.SharedGalleryDestination.SigDestinationResourceGroup, b.config.SharedGalleryDestination.SigDestinationImageName) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This change allows us to fail faster when the SIG version we're already publishing to exists, this would previously fail after the creation of the Build VM when trying to create the SIG version
| s.say("Capturing VHD ...") | ||
| err = s.captureVhd(ctx, vmId, vmCaptureParameters) | ||
| if err != nil { | ||
| state.Put(constants.Error, err) |
There was a problem hiding this comment.
This code is a bit messy now but I was having errors where the error was not correctly being passed into nested loops, so this felt simpler
JenGoldstrich
changed the title
| return nil | ||
| } | ||
|
|
||
| // HACK(chrboum): This method is a hack. It was written to work around this issue |
There was a problem hiding this comment.
Love it when we remove code that starts with a comment like this one :D
| vmId := vmResponse.Model.Properties.VMId | ||
| return *vmId, nil | ||
| } | ||
| return "", nil |
There was a problem hiding this comment.
I wonder when does that happen? Shouldn't that be an error if no ID is found in this case?
| return "", err | ||
| } | ||
| if vmResponse.Model != nil { | ||
| vmId := vmResponse.Model.Properties.VMId |
There was a problem hiding this comment.
Also, don't know how likely it is to happen, but can Properties be nil here? If so this will crash upon dereferencing it.
There was a problem hiding this comment.
TBH I think I need to standardize this across the plugin, null checking returned Models/properties from the new SDK, so I'd like to push off figuring that out to a later PR
| state.Put(constants.Error, err) | ||
| s.error(err) | ||
| return multistep.ActionHalt | ||
| } else { |
There was a problem hiding this comment.
Since the if ends with a return statement, you can remove this else and move the contents one level up
| state.Put(constants.Error, err) | ||
| s.error(err) | ||
| return multistep.ActionHalt | ||
| } else { |
There was a problem hiding this comment.
Same here, this else is unnecessary since the if ends with a return
Remove Template Capture, defeating a legacy TODO from 2016!
There's a lot of history here but the TL;Dr is we had to use TemplateCapture to get the
VMIDa UUID that is an internal ID that is used when capturing a VHD with the plugin. We can just query the VM API now though unlike in 2016 to get this ID, removing the need for this messy solution