From 67342750a374e89f064da834ad44618c0ff71e35 Mon Sep 17 00:00:00 2001 From: Amrita Dutta Date: Wed, 14 Nov 2018 01:47:48 +0000 Subject: [PATCH] Addressed PR comments --- builder/azure/arm/builder.go | 26 +++------- builder/azure/arm/config_test.go | 42 ++++++++++++++++ builder/azure/arm/step_snapshot_data_disks.go | 48 ++++++++++--------- .../arm/step_snapshot_data_disks_test.go | 10 ++-- builder/azure/arm/step_snapshot_os_disk.go | 43 +++++++++-------- .../azure/arm/step_snapshot_os_disk_test.go | 10 ++-- 6 files changed, 110 insertions(+), 69 deletions(-) diff --git a/builder/azure/arm/builder.go b/builder/azure/arm/builder.go index debc6108b..e278052f6 100644 --- a/builder/azure/arm/builder.go +++ b/builder/azure/arm/builder.go @@ -183,20 +183,13 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe NewStepGetOSDisk(azureClient, ui), NewStepGetAdditionalDisks(azureClient, ui), NewStepPowerOffCompute(azureClient, ui), - } - // if managed image create snapshot - if b.config.isManagedImage() { - steps = append(steps, - NewStepSnapshotOSDisk(azureClient, ui), - NewStepSnapshotDataDisks(azureClient, ui), - ) - } - steps = append(steps, + NewStepSnapshotOSDisk(azureClient, ui, b.config.isManagedImage()), + NewStepSnapshotDataDisks(azureClient, ui, b.config.isManagedImage()), NewStepCaptureImage(azureClient, ui), NewStepDeleteResourceGroup(azureClient, ui), NewStepDeleteOSDisk(azureClient, ui), NewStepDeleteAdditionalDisks(azureClient, ui), - ) + } } else if b.config.OSType == constants.Target_Windows { keyVaultDeploymentName := b.stateBag.Get(constants.ArmKeyVaultDeploymentName).(string) steps = []multistep.Step{ @@ -227,21 +220,14 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe &packerCommon.StepProvision{}, NewStepGetOSDisk(azureClient, ui), NewStepGetAdditionalDisks(azureClient, ui), - } - // if managed image create snapshot - if b.config.isManagedImage() { - steps = append(steps, - NewStepSnapshotOSDisk(azureClient, ui), - NewStepSnapshotDataDisks(azureClient, ui), - ) - } - steps = append(steps, + NewStepSnapshotOSDisk(azureClient, ui, b.config.isManagedImage()), + NewStepSnapshotDataDisks(azureClient, ui, b.config.isManagedImage()), NewStepPowerOffCompute(azureClient, ui), NewStepCaptureImage(azureClient, ui), NewStepDeleteResourceGroup(azureClient, ui), NewStepDeleteOSDisk(azureClient, ui), NewStepDeleteAdditionalDisks(azureClient, ui), - ) + } } else { return nil, fmt.Errorf("Builder does not support the os_type '%s'", b.config.OSType) } diff --git a/builder/azure/arm/config_test.go b/builder/azure/arm/config_test.go index e99761f84..1d2f711fd 100644 --- a/builder/azure/arm/config_test.go +++ b/builder/azure/arm/config_test.go @@ -912,6 +912,48 @@ func TestConfigShouldAcceptManagedImageOSDiskSnapshotNameAndManagedImageDataDisk } } +func TestConfigShouldRejectManagedImageOSDiskSnapshotNameAndManagedImageDataDiskSnapshotPrefixWithCaptureContainerName(t *testing.T) { + config := map[string]interface{}{ + "image_offer": "ignore", + "image_publisher": "ignore", + "image_sku": "ignore", + "location": "ignore", + "subscription_id": "ignore", + "communicator": "none", + "capture_container_name": "ignore", + "managed_image_os_disk_snapshot_name": "ignore_ignore", + "managed_image_data_disk_snapshot_prefix": "ignore_ignore", + // Does not matter for this test case, just pick one. + "os_type": constants.Target_Linux, + } + + _, _, err := newConfig(config, getPackerConfiguration()) + if err == nil { + t.Fatal("expected config to reject Managed Image build with data disk snapshot prefix and OS disk snapshot name with capture container name") + } +} + +func TestConfigShouldRejectManagedImageOSDiskSnapshotNameAndManagedImageDataDiskSnapshotPrefixWithCaptureNamePrefix(t *testing.T) { + config := map[string]interface{}{ + "image_offer": "ignore", + "image_publisher": "ignore", + "image_sku": "ignore", + "location": "ignore", + "subscription_id": "ignore", + "communicator": "none", + "capture_name_prefix": "ignore", + "managed_image_os_disk_snapshot_name": "ignore_ignore", + "managed_image_data_disk_snapshot_prefix": "ignore_ignore", + // Does not matter for this test case, just pick one. + "os_type": constants.Target_Linux, + } + + _, _, err := newConfig(config, getPackerConfiguration()) + if err == nil { + t.Fatal("expected config to reject Managed Image build with data disk snapshot prefix and OS disk snapshot name with capture name prefix") + } +} + func TestConfigShouldAcceptPlatformManagedImageBuild(t *testing.T) { config := map[string]interface{}{ "image_offer": "ignore", diff --git a/builder/azure/arm/step_snapshot_data_disks.go b/builder/azure/arm/step_snapshot_data_disks.go index 37f0342ac..baf56b443 100644 --- a/builder/azure/arm/step_snapshot_data_disks.go +++ b/builder/azure/arm/step_snapshot_data_disks.go @@ -13,17 +13,19 @@ import ( ) type StepSnapshotDataDisks struct { - client *AzureClient - create func(ctx context.Context, resourceGroupName string, srcUriVhd string, location string, tags map[string]*string, dstSnapshotName string) error - say func(message string) - error func(e error) + client *AzureClient + create func(ctx context.Context, resourceGroupName string, srcUriVhd string, location string, tags map[string]*string, dstSnapshotName string) error + say func(message string) + error func(e error) + isManagedImage bool } -func NewStepSnapshotDataDisks(client *AzureClient, ui packer.Ui) *StepSnapshotDataDisks { +func NewStepSnapshotDataDisks(client *AzureClient, ui packer.Ui, isManagedImage bool) *StepSnapshotDataDisks { var step = &StepSnapshotDataDisks{ - client: client, - say: func(message string) { ui.Say(message) }, - error: func(e error) { ui.Error(e.Error()) }, + client: client, + say: func(message string) { ui.Say(message) }, + error: func(e error) { ui.Error(e.Error()) }, + isManagedImage: isManagedImage, } step.create = step.createDataDiskSnapshot @@ -70,25 +72,27 @@ func (s *StepSnapshotDataDisks) createDataDiskSnapshot(ctx context.Context, reso } func (s *StepSnapshotDataDisks) Run(ctx context.Context, stateBag multistep.StateBag) multistep.StepAction { - s.say("Taking snapshot of OS disk ...") + if s.isManagedImage { - var resourceGroupName = stateBag.Get(constants.ArmManagedImageResourceGroupName).(string) - var location = stateBag.Get(constants.ArmLocation).(string) - var tags = stateBag.Get(constants.ArmTags).(map[string]*string) - var additionalDisks = stateBag.Get(constants.ArmAdditionalDiskVhds).([]string) - var dstSnapshotPrefix = stateBag.Get(constants.ArmManagedImageDataDiskSnapshotPrefix).(string) + s.say("Taking snapshot of data disk ...") - for i, disk := range additionalDisks { - dstSnapshotName := dstSnapshotPrefix + strconv.Itoa(i) - err := s.create(ctx, resourceGroupName, disk, location, tags, dstSnapshotName) + var resourceGroupName = stateBag.Get(constants.ArmManagedImageResourceGroupName).(string) + var location = stateBag.Get(constants.ArmLocation).(string) + var tags = stateBag.Get(constants.ArmTags).(map[string]*string) + var additionalDisks = stateBag.Get(constants.ArmAdditionalDiskVhds).([]string) + var dstSnapshotPrefix = stateBag.Get(constants.ArmManagedImageDataDiskSnapshotPrefix).(string) - if err != nil { - stateBag.Put(constants.Error, err) - s.error(err) + for i, disk := range additionalDisks { + dstSnapshotName := dstSnapshotPrefix + strconv.Itoa(i) + err := s.create(ctx, resourceGroupName, disk, location, tags, dstSnapshotName) - return multistep.ActionHalt + if err != nil { + stateBag.Put(constants.Error, err) + s.error(err) + + return multistep.ActionHalt + } } - } return multistep.ActionContinue diff --git a/builder/azure/arm/step_snapshot_data_disks_test.go b/builder/azure/arm/step_snapshot_data_disks_test.go index aee7523ec..e7203aade 100644 --- a/builder/azure/arm/step_snapshot_data_disks_test.go +++ b/builder/azure/arm/step_snapshot_data_disks_test.go @@ -13,8 +13,9 @@ func TestStepSnapshotDataDisksShouldFailIfSnapshotFails(t *testing.T) { create: func(context.Context, string, string, string, map[string]*string, string) error { return fmt.Errorf("!! Unit Test FAIL !!") }, - say: func(message string) {}, - error: func(e error) {}, + say: func(message string) {}, + error: func(e error) {}, + isManagedImage: true, } stateBag := createTestStateBagStepSnapshotDataDisks() @@ -34,8 +35,9 @@ func TestStepSnapshotDataDisksShouldPassIfSnapshotPasses(t *testing.T) { create: func(context.Context, string, string, string, map[string]*string, string) error { return nil }, - say: func(message string) {}, - error: func(e error) {}, + say: func(message string) {}, + error: func(e error) {}, + isManagedImage: true, } stateBag := createTestStateBagStepSnapshotDataDisks() diff --git a/builder/azure/arm/step_snapshot_os_disk.go b/builder/azure/arm/step_snapshot_os_disk.go index ebb5e0486..7f349c07e 100644 --- a/builder/azure/arm/step_snapshot_os_disk.go +++ b/builder/azure/arm/step_snapshot_os_disk.go @@ -11,17 +11,19 @@ import ( ) type StepSnapshotOSDisk struct { - client *AzureClient - create func(ctx context.Context, resourceGroupName string, srcUriVhd string, location string, tags map[string]*string, dstSnapshotName string) error - say func(message string) - error func(e error) + client *AzureClient + create func(ctx context.Context, resourceGroupName string, srcUriVhd string, location string, tags map[string]*string, dstSnapshotName string) error + say func(message string) + error func(e error) + isManagedImage bool } -func NewStepSnapshotOSDisk(client *AzureClient, ui packer.Ui) *StepSnapshotOSDisk { +func NewStepSnapshotOSDisk(client *AzureClient, ui packer.Ui, isManagedImage bool) *StepSnapshotOSDisk { var step = &StepSnapshotOSDisk{ - client: client, - say: func(message string) { ui.Say(message) }, - error: func(e error) { ui.Error(e.Error()) }, + client: client, + say: func(message string) { ui.Say(message) }, + error: func(e error) { ui.Error(e.Error()) }, + isManagedImage: isManagedImage, } step.create = step.createSnapshot @@ -68,21 +70,24 @@ func (s *StepSnapshotOSDisk) createSnapshot(ctx context.Context, resourceGroupNa } func (s *StepSnapshotOSDisk) Run(ctx context.Context, stateBag multistep.StateBag) multistep.StepAction { - s.say("Taking snapshot of OS disk ...") + if s.isManagedImage { - var resourceGroupName = stateBag.Get(constants.ArmManagedImageResourceGroupName).(string) - var location = stateBag.Get(constants.ArmLocation).(string) - var tags = stateBag.Get(constants.ArmTags).(map[string]*string) - var srcUriVhd = stateBag.Get(constants.ArmOSDiskVhd).(string) - var dstSnapshotName = stateBag.Get(constants.ArmManagedImageOSDiskSnapshotName).(string) + s.say("Taking snapshot of OS disk ...") - err := s.create(ctx, resourceGroupName, srcUriVhd, location, tags, dstSnapshotName) + var resourceGroupName = stateBag.Get(constants.ArmManagedImageResourceGroupName).(string) + var location = stateBag.Get(constants.ArmLocation).(string) + var tags = stateBag.Get(constants.ArmTags).(map[string]*string) + var srcUriVhd = stateBag.Get(constants.ArmOSDiskVhd).(string) + var dstSnapshotName = stateBag.Get(constants.ArmManagedImageOSDiskSnapshotName).(string) - if err != nil { - stateBag.Put(constants.Error, err) - s.error(err) + err := s.create(ctx, resourceGroupName, srcUriVhd, location, tags, dstSnapshotName) - return multistep.ActionHalt + if err != nil { + stateBag.Put(constants.Error, err) + s.error(err) + + return multistep.ActionHalt + } } return multistep.ActionContinue diff --git a/builder/azure/arm/step_snapshot_os_disk_test.go b/builder/azure/arm/step_snapshot_os_disk_test.go index f4e7eeafb..b7342319b 100644 --- a/builder/azure/arm/step_snapshot_os_disk_test.go +++ b/builder/azure/arm/step_snapshot_os_disk_test.go @@ -13,8 +13,9 @@ func TestStepSnapshotOSDiskShouldFailIfSnapshotFails(t *testing.T) { create: func(context.Context, string, string, string, map[string]*string, string) error { return fmt.Errorf("!! Unit Test FAIL !!") }, - say: func(message string) {}, - error: func(e error) {}, + say: func(message string) {}, + error: func(e error) {}, + isManagedImage: true, } stateBag := createTestStateBagStepSnapshotOSDisk() @@ -34,8 +35,9 @@ func TestStepSnapshotOSDiskShouldPassIfSnapshotPasses(t *testing.T) { create: func(context.Context, string, string, string, map[string]*string, string) error { return nil }, - say: func(message string) {}, - error: func(e error) {}, + say: func(message string) {}, + error: func(e error) {}, + isManagedImage: true, } stateBag := createTestStateBagStepSnapshotOSDisk()