From 305592d8edfb498b1e456d96dd9f6e7c16441d90 Mon Sep 17 00:00:00 2001
From: Megan Marsh <megan@hashicorp.com>
Date: Mon, 17 Jun 2019 15:38:28 -0700
Subject: [PATCH] fix copy logic and tests

---
 builder/amazon/common/step_ami_region_copy.go | 30 ++++++++++++-------
 .../common/step_ami_region_copy_test.go       | 10 +++----
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/builder/amazon/common/step_ami_region_copy.go b/builder/amazon/common/step_ami_region_copy.go
index bfc582e5a..4a1f2d392 100644
--- a/builder/amazon/common/step_ami_region_copy.go
+++ b/builder/amazon/common/step_ami_region_copy.go
@@ -33,19 +33,25 @@ func (s *StepAMIRegionCopy) Run(ctx context.Context, state multistep.StateBag) m
 
 	ami := amis[s.OriginalRegion]
 	// Always copy back into original region to preserve the ami name
-	s.toDelete = ami
-	if !s.AMISkipBuildRegion {
-		s.Regions = append(s.Regions, s.OriginalRegion)
+	if s.EncryptBootVolume != nil || s.AMISkipBuildRegion {
+		// if we haven't specificed encryption and we aren't skipping the save
+		// to the build region, we don't have anything to delete
+		s.toDelete = ami
 	}
 
-	if s.EncryptBootVolume != nil && *s.EncryptBootVolume {
-		// encrypt_boot is true, so we have to copy the temporary
-		// AMI with required encryption setting.
-		// temp image was created by stepCreateAMI.
-		if s.RegionKeyIds == nil {
-			s.RegionKeyIds = make(map[string]string)
+	if s.EncryptBootVolume != nil {
+		if !s.AMISkipBuildRegion {
+			s.Regions = append(s.Regions, s.OriginalRegion)
+		}
+		if *s.EncryptBootVolume {
+			// encrypt_boot is true, so we have to copy the temporary
+			// AMI with required encryption setting.
+			// temp image was created by stepCreateAMI.
+			if s.RegionKeyIds == nil {
+				s.RegionKeyIds = make(map[string]string)
+			}
+			s.RegionKeyIds[s.OriginalRegion] = s.AMIKmsKeyId
 		}
-		s.RegionKeyIds[s.OriginalRegion] = s.AMIKmsKeyId
 	}
 
 	if len(s.Regions) == 0 {
@@ -99,6 +105,10 @@ func (s *StepAMIRegionCopy) Cleanup(state multistep.StateBag) {
 	ec2conn := state.Get("ec2").(*ec2.EC2)
 	ui := state.Get("ui").(packer.Ui)
 
+	if len(s.toDelete) == 0 {
+		return
+	}
+
 	// Delete the unencrypted amis and snapshots
 	ui.Say("Deregistering the AMI and deleting unencrypted temporary " +
 		"AMIs and snapshots")
diff --git a/builder/amazon/common/step_ami_region_copy_test.go b/builder/amazon/common/step_ami_region_copy_test.go
index 26f35f7b4..03841b67d 100644
--- a/builder/amazon/common/step_ami_region_copy_test.go
+++ b/builder/amazon/common/step_ami_region_copy_test.go
@@ -101,11 +101,11 @@ func TestStepAmiRegionCopy_nil_encryption(t *testing.T) {
 	state := tState()
 	stepAMIRegionCopy.Run(context.Background(), state)
 
-	if stepAMIRegionCopy.toDelete != "ami-12345" {
-		t.Fatalf("Should delete original intermediary ami even if not encrypted")
+	if stepAMIRegionCopy.toDelete != "" {
+		t.Fatalf("Shouldn't have an intermediary ami if encrypt is nil")
 	}
-	if len(stepAMIRegionCopy.Regions) == 0 {
-		t.Fatalf("Should have added original ami to original region")
+	if len(stepAMIRegionCopy.Regions) != 0 {
+		t.Fatalf("Should not have added original ami to original region")
 	}
 }
 
@@ -179,7 +179,7 @@ func TestStepAmiRegionCopy_true_AMISkipBuildRegion(t *testing.T) {
 	if stepAMIRegionCopy.toDelete == "" {
 		t.Fatalf("Should delete original AMI if skip_save_build_region=true")
 	}
-	if len(stepAMIRegionCopy.Regions) == 0 {
+	if len(stepAMIRegionCopy.Regions) != 0 {
 		t.Fatalf("Should not have added original ami to Regions")
 	}
 }