builder/docker: change commit to the opt-in, special case build steps

/cc @andytson - Wanted to CC you in here so you could see some changes I
made.

First, I changed commit to opt-in, so you set "commit": true to get
commit behavior. This simplifies the logic a bit. Then, I removed the
skipping for StepExport and StepCommit and put that into the Builder
itself. This simplifies those steps (limits abstraction leakage).
Otherwise, everything looks great!

builder/docker/builder.go

@@ -8,6 +8,7 @@ import (
 )
 
 const BuilderId = "packer.docker"
+const BuilderIdImport = "packer.post-processor.docker-import"
 
 type Builder struct {
 	config *Config
@@ -35,8 +36,12 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe
 		&StepPull{},
 		&StepRun{},
 		&StepProvision{},
-		&StepCommit{},
-		&StepExport{},
+	}
+
+	if b.config.Commit {
+		steps = append(steps, new(StepCommit))
+	} else {
+		steps = append(steps, new(StepExport))
 	}
 
 	// Setup the state bag and initial state for the steps
@@ -67,14 +72,14 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe
 
 	var artifact packer.Artifact
 	// No errors, must've worked
-	if b.config.Export {
-		artifact = &ExportArtifact{path: b.config.ExportPath}
-	} else {
+	if b.config.Commit {
 		artifact = &ImportArtifact{
 			IdValue:        state.Get("image_id").(string),
-			BuilderIdValue: "packer.post-processor.docker-import",
+			BuilderIdValue: BuilderIdImport,
 			Driver:         driver,
 		}
+	} else {
+		artifact = &ExportArtifact{path: b.config.ExportPath}
 	}
 	return artifact, nil
 }

builder/docker/config.go

@@ -9,8 +9,8 @@ import (
 type Config struct {
 	common.PackerConfig `mapstructure:",squash"`
 
+	Commit     bool
 	ExportPath string `mapstructure:"export_path"`
-	Export     bool
 	Image      string
 	Pull       bool
 	RunCommand []string `mapstructure:"run_command"`
@@ -72,13 +72,16 @@ func NewConfig(raws ...interface{}) (*Config, []string, error) {
 		}
 	}
 
-	c.Export = c.ExportPath != ""
-
 	if c.Image == "" {
 		errs = packer.MultiErrorAppend(errs,
 			fmt.Errorf("image must be specified"))
 	}
 
+	if c.ExportPath != "" && c.Commit {
+		errs = packer.MultiErrorAppend(errs,
+			fmt.Errorf("both commit and export_path cannot be set"))
+	}
+
 	if errs != nil && len(errs.Errors) > 0 {
 		return nil, nil, errs
 	}

builder/docker/config_test.go

@@ -46,19 +46,27 @@ func TestConfigPrepare_exportPath(t *testing.T) {
 
 	// No export path
 	delete(raw, "export_path")
-	c, warns, errs := NewConfig(raw)
+	_, warns, errs := NewConfig(raw)
 	testConfigOk(t, warns, errs)
-	if c.Export {
-		t.Fatal("should not export")
-	}
 
 	// Good export path
 	raw["export_path"] = "good"
-	c, warns, errs = NewConfig(raw)
+	_, warns, errs = NewConfig(raw)
+	testConfigOk(t, warns, errs)
+}
+
+func TestConfigPrepare_exportPathAndCommit(t *testing.T) {
+	raw := testConfig()
+	raw["commit"] = true
+
+	// No export path
+	_, warns, errs := NewConfig(raw)
+	testConfigErr(t, warns, errs)
+
+	// No commit
+	raw["commit"] = false
+	_, warns, errs = NewConfig(raw)
 	testConfigOk(t, warns, errs)
-	if !c.Export {
-		t.Fatal("should export")
-	}
 }
 
 func TestConfigPrepare_image(t *testing.T) {

builder/docker/step_commit.go

@@ -12,15 +12,10 @@ type StepCommit struct {
 }
 
 func (s *StepCommit) Run(state multistep.StateBag) multistep.StepAction {
-	config := state.Get("config").(*Config)
 	driver := state.Get("driver").(Driver)
 	containerId := state.Get("container_id").(string)
 	ui := state.Get("ui").(packer.Ui)
 
-	if config.Export {
-		return multistep.ActionContinue
-	}
-
 	ui.Say("Committing the container")
 	imageId, err := driver.Commit(containerId)
 	if err != nil {

builder/docker/step_commit_test.go

@@ -21,8 +21,6 @@ func TestStepCommit(t *testing.T) {
 	step := new(StepCommit)
 	defer step.Cleanup(state)
 
-	config := state.Get("config").(*Config)
-	config.Export = false
 	driver := state.Get("driver").(*MockDriver)
 	driver.CommitImageId = "bar"
 
@@ -48,38 +46,11 @@ func TestStepCommit(t *testing.T) {
 	}
 }
 
-func TestStepCommit_skip(t *testing.T) {
-	state := testStepCommitState(t)
-	step := new(StepCommit)
-	defer step.Cleanup(state)
-
-	config := state.Get("config").(*Config)
-	config.Export = true
-	driver := state.Get("driver").(*MockDriver)
-
-	// run the step
-	if action := step.Run(state); action != multistep.ActionContinue {
-		t.Fatalf("bad action: %#v", action)
-	}
-
-	// verify we did the right thing
-	if driver.CommitCalled {
-		t.Fatal("shouldn't have called")
-	}
-
-	// verify the ID is not saved
-	if _, ok := state.GetOk("image_id"); ok {
-		t.Fatal("shouldn't save image ID")
-	}
-}
-
 func TestStepCommit_error(t *testing.T) {
 	state := testStepCommitState(t)
 	step := new(StepCommit)
 	defer step.Cleanup(state)
 
-	config := state.Get("config").(*Config)
-	config.Export = false
 	driver := state.Get("driver").(*MockDriver)
 	driver.CommitErr = errors.New("foo")
 

builder/docker/step_export.go

@@ -13,10 +13,6 @@ type StepExport struct{}
 func (s *StepExport) Run(state multistep.StateBag) multistep.StepAction {
 	config := state.Get("config").(*Config)
 
-	if !config.Export {
-		return multistep.ActionContinue
-	}
-
 	driver := state.Get("driver").(Driver)
 	containerId := state.Get("container_id").(string)
 	ui := state.Get("ui").(packer.Ui)

builder/docker/step_export_test.go

@@ -34,7 +34,6 @@ func TestStepExport(t *testing.T) {
 
 	config := state.Get("config").(*Config)
 	config.ExportPath = tf.Name()
-	config.Export = true
 	driver := state.Get("driver").(*MockDriver)
 	driver.ExportReader = bytes.NewReader([]byte("data!"))
 
@@ -62,26 +61,6 @@ func TestStepExport(t *testing.T) {
 	}
 }
 
-func TestStepExport_skip(t *testing.T) {
-	state := testStepExportState(t)
-	step := new(StepExport)
-	defer step.Cleanup(state)
-
-	config := state.Get("config").(*Config)
-	config.Export = false
-	driver := state.Get("driver").(*MockDriver)
-
-	// run the step
-	if action := step.Run(state); action != multistep.ActionContinue {
-		t.Fatalf("bad action: %#v", action)
-	}
-
-	// verify we did the right thing
-	if driver.ExportCalled {
-		t.Fatal("shouldn't have exported")
-	}
-}
-
 func TestStepExport_error(t *testing.T) {
 	state := testStepExportState(t)
 	step := new(StepExport)
@@ -100,7 +79,6 @@ func TestStepExport_error(t *testing.T) {
 
 	config := state.Get("config").(*Config)
 	config.ExportPath = tf.Name()
-	config.Export = true
 	driver := state.Get("driver").(*MockDriver)
 	driver.ExportError = errors.New("foo")