Process review feedback

builder/azure/arm/builder_acc_test.go

@@ -18,7 +18,7 @@ package arm
 // a non-empty value to enable Packer acceptance tests and the
 // options "-v -timeout 30m" should be provided to the test
 // command, e.g.:
-//   go test -v -timeout 30m -run TestBuilderAcc_.*
+//   go test -v -timeout 90m -run TestBuilderAcc_.*
 
 import (
 	"testing"

builder/azure/arm/step.go

@@ -1,20 +1,10 @@
 package arm
 
 import (
-	"github.com/hashicorp/packer/builder/azure/common"
 	"github.com/hashicorp/packer/builder/azure/common/constants"
 	"github.com/hashicorp/packer/helper/multistep"
 )
 
-func processInterruptibleResult(
-	result common.InterruptibleTaskResult, sayError func(error), state multistep.StateBag) multistep.StepAction {
-	if result.IsCancelled {
-		return multistep.ActionHalt
-	}
-
-	return processStepResult(result.Err, sayError, state)
-}
-
 func processStepResult(
 	err error, sayError func(error), state multistep.StateBag) multistep.StepAction {
 

builder/azure/arm/step_capture_image.go

@@ -103,9 +103,7 @@ func (s *StepCaptureImage) Run(ctx context.Context, state multistep.StateBag) mu
 	}
 
 	// HACK(chrboum): I do not like this.  The capture method should be returning this value
-	// instead having to pass in another lambda.  I'm in this pickle because I am using
+	// instead having to pass in another lambda.
-	// common.StartInterruptibleTask which is not parametric, and only returns a type of error.
-	// I could change it to interface{}, but I do not like that solution either.
 	//
 	// Having to resort to capturing the template via an inspector is hack, and once I can
 	// resolve that I can cleanup this code too.  See the comments in azure_client.go for more

builder/azure/arm/step_delete_additional_disks.go

@@ -43,10 +43,9 @@ func (s *StepDeleteAdditionalDisk) deleteBlob(storageContainerName string, blobN
 	return err
 }
 
-func (s *StepDeleteAdditionalDisk) deleteManagedDisk(ctx context.Context, resourceGroupName string, imageName string) error {
+func (s *StepDeleteAdditionalDisk) deleteManagedDisk(ctx context.Context, resourceGroupName string, diskName string) error {
-	xs := strings.Split(imageName, "/")
+	xs := strings.Split(diskName, "/")
-	// TODO(paulmey): verify if this is right, imagename is passed in, *disk* is deleted... is this a copy/paste error?
+	diskName = xs[len(xs)-1]
-	diskName := xs[len(xs)-1]
 	f, err := s.client.DisksClient.Delete(ctx, resourceGroupName, diskName)
 	if err == nil {
 		err = f.WaitForCompletion(ctx, s.client.DisksClient.Client)

builder/azure/arm/step_test.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/hashicorp/packer/builder/azure/common"
 	"github.com/hashicorp/packer/builder/azure/common/constants"
 	"github.com/hashicorp/packer/helper/multistep"
 )
@@ -39,59 +38,3 @@ func TestProcessStepResultShouldHaltOnError(t *testing.T) {
 		t.Errorf("Expected ActionHalt(%d), but got=%d", multistep.ActionHalt, code)
 	}
 }
-
-func TestProcessStepResultShouldContinueOnSuccessfulTask(t *testing.T) {
-	stateBag := new(multistep.BasicStateBag)
-	result := common.InterruptibleTaskResult{
-		IsCancelled: false,
-		Err:         nil,
-	}
-
-	code := processInterruptibleResult(result, func(error) { t.Fatal("Should not be called!") }, stateBag)
-	if _, ok := stateBag.GetOk(constants.Error); ok {
-		t.Errorf("Error was nil, but was still in the state bag.")
-	}
-
-	if code != multistep.ActionContinue {
-		t.Errorf("Expected ActionContinue(%d), but got=%d", multistep.ActionContinue, code)
-	}
-}
-
-func TestProcessStepResultShouldHaltWhenTaskIsCancelled(t *testing.T) {
-	stateBag := new(multistep.BasicStateBag)
-	result := common.InterruptibleTaskResult{
-		IsCancelled: true,
-		Err:         nil,
-	}
-
-	code := processInterruptibleResult(result, func(error) { t.Fatal("Should not be called!") }, stateBag)
-	if _, ok := stateBag.GetOk(constants.Error); ok {
-		t.Errorf("Error was nil, but was still in the state bag.")
-	}
-
-	if code != multistep.ActionHalt {
-		t.Errorf("Expected ActionHalt(%d), but got=%d", multistep.ActionHalt, code)
-	}
-}
-
-func TestProcessStepResultShouldHaltOnTaskError(t *testing.T) {
-	stateBag := new(multistep.BasicStateBag)
-	isSaidError := false
-	result := common.InterruptibleTaskResult{
-		IsCancelled: false,
-		Err:         fmt.Errorf("boom"),
-	}
-
-	code := processInterruptibleResult(result, func(error) { isSaidError = true }, stateBag)
-	if _, ok := stateBag.GetOk(constants.Error); !ok {
-		t.Errorf("Error was *not* nil, but was not in the state bag.")
-	}
-
-	if !isSaidError {
-		t.Errorf("Expected error to be said, but it was not.")
-	}
-
-	if code != multistep.ActionHalt {
-		t.Errorf("Expected ActionHalt(%d), but got=%d", multistep.ActionHalt, code)
-	}
-}

builder/azure/common/interruptible_task.go

@@ -1,54 +0,0 @@
-package common
-
-import (
-	"time"
-)
-
-type InterruptibleTaskResult struct {
-	Err         error
-	IsCancelled bool
-}
-
-type InterruptibleTask struct {
-	IsCancelled func() bool
-	Task        func(cancelCh <-chan struct{}) error
-}
-
-func NewInterruptibleTask(isCancelled func() bool, task func(cancelCh <-chan struct{}) error) *InterruptibleTask {
-	return &InterruptibleTask{
-		IsCancelled: isCancelled,
-		Task:        task,
-	}
-}
-
-func StartInterruptibleTask(isCancelled func() bool, task func(cancelCh <-chan struct{}) error) InterruptibleTaskResult {
-	t := NewInterruptibleTask(isCancelled, task)
-	return t.Run()
-}
-
-func (s *InterruptibleTask) Run() InterruptibleTaskResult {
-	completeCh := make(chan error)
-
-	cancelCh := make(chan struct{})
-	defer close(cancelCh)
-
-	go func() {
-		err := s.Task(cancelCh)
-		completeCh <- err
-
-		// senders close, receivers check for close
-		close(completeCh)
-	}()
-
-	for {
-		if s.IsCancelled() {
-			return InterruptibleTaskResult{Err: nil, IsCancelled: true}
-		}
-
-		select {
-		case err := <-completeCh:
-			return InterruptibleTaskResult{Err: err, IsCancelled: false}
-		case <-time.After(100 * time.Millisecond):
-		}
-	}
-}

builder/azure/common/interruptible_task_test.go

@@ -1,102 +0,0 @@
-package common
-
-import (
-	"fmt"
-	"testing"
-	"time"
-)
-
-func TestInterruptibleTaskShouldImmediatelyEndOnCancel(t *testing.T) {
-	testSubject := NewInterruptibleTask(
-		func() bool { return true },
-		func(<-chan struct{}) error {
-			for {
-				time.Sleep(time.Second * 30)
-			}
-		})
-
-	result := testSubject.Run()
-	if result.IsCancelled != true {
-		t.Fatal("Expected the task to be cancelled, but it was not.")
-	}
-}
-
-func TestInterruptibleTaskShouldRunTaskUntilCompletion(t *testing.T) {
-	var count int
-
-	testSubject := &InterruptibleTask{
-		IsCancelled: func() bool {
-			return false
-		},
-		Task: func(<-chan struct{}) error {
-			for i := 0; i < 10; i++ {
-				count += 1
-			}
-
-			return nil
-		},
-	}
-
-	result := testSubject.Run()
-	if result.IsCancelled != false {
-		t.Errorf("Expected the task to *not* be cancelled, but it was.")
-	}
-
-	if count != 10 {
-		t.Errorf("Expected the task to wait for completion, but it did not.")
-	}
-
-	if result.Err != nil {
-		t.Errorf("Expected the task to return a nil error, but got=%s", result.Err)
-	}
-}
-
-func TestInterruptibleTaskShouldImmediatelyStopOnTaskError(t *testing.T) {
-	testSubject := &InterruptibleTask{
-		IsCancelled: func() bool {
-			return false
-		},
-		Task: func(cancelCh <-chan struct{}) error {
-			return fmt.Errorf("boom")
-		},
-	}
-
-	result := testSubject.Run()
-	if result.IsCancelled != false {
-		t.Errorf("Expected the task to *not* be cancelled, but it was.")
-	}
-
-	if result.Err == nil {
-		t.Errorf("Expected the task to return an error, but it did not.")
-	}
-}
-
-func TestInterruptibleTaskShouldProvideLiveChannel(t *testing.T) {
-	testSubject := &InterruptibleTask{
-		IsCancelled: func() bool {
-			return false
-		},
-		Task: func(cancelCh <-chan struct{}) error {
-			isOpen := false
-
-			select {
-			case _, ok := <-cancelCh:
-				isOpen = !ok
-				if !isOpen {
-					t.Errorf("Expected the channel to open, but it was closed.")
-				}
-			default:
-				isOpen = true
-				break
-			}
-
-			if !isOpen {
-				t.Errorf("Check for openness failed.")
-			}
-
-			return nil
-		},
-	}
-
-	testSubject.Run()
-}