packer/plugin: Refactor to get interfaces from Client

packer/plugin/builder.go

@@ -2,10 +2,7 @@ package plugin
 
 import (
 	"github.com/mitchellh/packer/packer"
-	packrpc "github.com/mitchellh/packer/packer/rpc"
 	"log"
-	"net/rpc"
-	"os/exec"
 )
 
 type cmdBuilder struct {
@@ -47,43 +44,3 @@ func (c *cmdBuilder) checkExit(p interface{}, cb func()) {
 		log.Panic(p)
 	}
 }
-
-// Returns a valid packer.Builder where the builder is executed via RPC
-// to a plugin that is within a subprocess.
-//
-// This method will start the given exec.Cmd, which should point to
-// the plugin binary to execute. Some configuration will be done to
-// the command, such as overriding Stdout and some environmental variables.
-//
-// This function guarantees the subprocess will end in a timely manner.
-func Builder(cmd *exec.Cmd) (result packer.Builder, err error) {
-	config := &ClientConfig{
-		Cmd: cmd,
-		Managed: true,
-	}
-
-	cmdClient := NewClient(config)
-	address, err := cmdClient.Start()
-	if err != nil {
-		return
-	}
-
-	defer func() {
-		// Make sure the command is properly killed in the case of an error
-		if err != nil {
-			cmdClient.Kill()
-		}
-	}()
-
-	client, err := rpc.Dial("tcp", address)
-	if err != nil {
-		return
-	}
-
-	result = &cmdBuilder{
-		packrpc.Builder(client),
-		cmdClient,
-	}
-
-	return
-}

packer/plugin/builder_test.go

@@ -1,7 +1,6 @@
 package plugin
 
 import (
-	"cgl.tideland.biz/asserts"
 	"github.com/mitchellh/packer/packer"
 	"os/exec"
 	"testing"
@@ -20,15 +19,21 @@ func (helperBuilder) Run(packer.Ui, packer.Hook, packer.Cache) packer.Artifact {
 func (helperBuilder) Cancel() {}
 
 func TestBuilder_NoExist(t *testing.T) {
-	assert := asserts.NewTestingAsserts(t, true)
+	c := NewClient(&ClientConfig{Cmd: exec.Command("i-should-not-exist")})
+	defer c.Kill()
 
-	_, err := Builder(exec.Command("i-should-never-ever-ever-exist"))
-	assert.NotNil(err, "should have an error")
+	_, err := c.Builder()
+	if err == nil {
+		t.Fatal("should have error")
+	}
 }
 
 func TestBuilder_Good(t *testing.T) {
-	assert := asserts.NewTestingAsserts(t, true)
+	c := NewClient(&ClientConfig{Cmd: helperProcess("builder")})
+	defer c.Kill()
 
-	_, err := Builder(helperProcess("builder"))
-	assert.Nil(err, "should start builder properly")
+	_, err := c.Builder()
+	if err != nil {
+		t.Fatalf("should not have error: %s", err)
+	}
 }

packer/plugin/client.go

@@ -4,8 +4,11 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
+	"github.com/mitchellh/packer/packer"
+	packrpc "github.com/mitchellh/packer/packer/rpc"
 	"io"
 	"log"
+	"net/rpc"
 	"os"
 	"os/exec"
 	"strings"
@@ -20,9 +23,9 @@ var managedClients = make([]*client, 0, 5)
 type client struct {
 	config *ClientConfig
 	exited      bool
-	started     bool
-	startL      sync.Mutex
 	doneLogging bool
+	l           sync.Mutex
+	address     string
 }
 
 // ClientConfig is the configuration used to initialize a new
@@ -101,6 +104,50 @@ func (c *client) Exited() bool {
 	return c.exited
 }
 
+// Returns a builder implementation that is communicating over this
+// client. If the client hasn't been started, this will start it.
+func (c *client) Builder() (packer.Builder, error) {
+	client, err := c.rpcClient()
+	if err != nil {
+		return nil, err
+	}
+
+	return &cmdBuilder{packrpc.Builder(client), c}, nil
+}
+
+// Returns a command implementation that is communicating over this
+// client. If the client hasn't been started, this will start it.
+func (c *client) Command() (packer.Command, error) {
+	client, err := c.rpcClient()
+	if err != nil {
+		return nil, err
+	}
+
+	return &cmdCommand{packrpc.Command(client), c}, nil
+}
+
+// Returns a hook implementation that is communicating over this
+// client. If the client hasn't been started, this will start it.
+func (c *client) Hook() (packer.Hook, error) {
+	client, err := c.rpcClient()
+	if err != nil {
+		return nil, err
+	}
+
+	return &cmdHook{packrpc.Hook(client), c}, nil
+}
+
+// Returns a provisioner implementation that is communicating over this
+// client. If the client hasn't been started, this will start it.
+func (c *client) Provisioner() (packer.Provisioner, error) {
+	client, err := c.rpcClient()
+	if err != nil {
+		return nil, err
+	}
+
+	return &cmdProvisioner{packrpc.Provisioner(client), c}, nil
+}
+
 // End the executing subprocess (if it is running) and perform any cleanup
 // tasks necessary such as capturing any remaining logs and so on.
 //
@@ -136,13 +183,12 @@ func (c *client) Kill() {
 // Once a client has been started once, it cannot be started again, even if
 // it was killed.
 func (c *client) Start() (address string, err error) {
-	c.startL.Lock()
-	defer c.startL.Unlock()
+	c.l.Lock()
+	defer c.l.Unlock()
 
-	if c.started {
-		panic("plugin client already started once")
+	if c.address != "" {
+		return c.address, nil
 	}
-	c.started = true
 
 	env := []string{
 		fmt.Sprintf("PACKER_PLUGIN_MIN_PORT=%d", c.config.MinPort),
@@ -205,7 +251,8 @@ func (c *client) Start() (address string, err error) {
 		if line, lerr := stdout.ReadBytes('\n'); lerr == nil {
 			// Trim the address and reset the err since we were able
 			// to read some sort of address.
-			address = strings.TrimSpace(string(line))
+			c.address = strings.TrimSpace(string(line))
+			address = c.address
 			err = nil
 			break
 		}
@@ -243,3 +290,17 @@ func (c *client) logStderr(buf *bytes.Buffer) {
 	// Flag that we've completed logging for others
 	c.doneLogging = true
 }
+
+func (c *client) rpcClient() (*rpc.Client, error) {
+	address, err := c.Start()
+	if err != nil {
+		return nil, err
+	}
+
+	client, err := rpc.Dial("tcp", address)
+	if err != nil {
+		return nil, err
+	}
+
+	return client, nil
+}

packer/plugin/client_test.go

@@ -33,22 +33,6 @@ func TestClient(t *testing.T) {
 	}
 }
 
-func TestClient_Start_Once(t *testing.T) {
-	process := helperProcess("mock")
-	c := NewClient(&ClientConfig{Cmd: process})
-	defer c.Kill()
-
-	defer func() {
-		p := recover()
-		if p == nil {
-			t.Fatal("should've paniced")
-		}
-	}()
-
-	c.Start()
-	c.Start()
-}
-
 func TestClient_Start_Timeout(t *testing.T) {
 	config := &ClientConfig{
 		Cmd: helperProcess("start-timeout"),

packer/plugin/command.go

@@ -2,10 +2,7 @@ package plugin
 
 import (
 	"github.com/mitchellh/packer/packer"
-	packrpc "github.com/mitchellh/packer/packer/rpc"
 	"log"
-	"net/rpc"
-	"os/exec"
 )
 
 type cmdCommand struct {
@@ -52,43 +49,3 @@ func (c *cmdCommand) checkExit(p interface{}, cb func()) {
 		log.Panic(p)
 	}
 }
-
-// Returns a valid packer.Command where the command is executed via RPC
-// to a plugin that is within a subprocess.
-//
-// This method will start the given exec.Cmd, which should point to
-// the plugin binary to execute. Some configuration will be done to
-// the command, such as overriding Stdout and some environmental variables.
-//
-// This function guarantees the subprocess will end in a timely manner.
-func Command(cmd *exec.Cmd) (result packer.Command, err error) {
-	config := &ClientConfig{
-		Cmd: cmd,
-		Managed: true,
-	}
-
-	cmdClient := NewClient(config)
-	address, err := cmdClient.Start()
-	if err != nil {
-		return
-	}
-
-	defer func() {
-		// Make sure the command is properly killed in the case of an error
-		if err != nil {
-			cmdClient.Kill()
-		}
-	}()
-
-	client, err := rpc.Dial("tcp", address)
-	if err != nil {
-		return
-	}
-
-	result = &cmdCommand{
-		packrpc.Command(client),
-		cmdClient,
-	}
-
-	return
-}

packer/plugin/command_test.go

@@ -1,7 +1,6 @@
 package plugin
 
 import (
-	"cgl.tideland.biz/asserts"
 	"github.com/mitchellh/packer/packer"
 	"os/exec"
 	"testing"
@@ -22,40 +21,31 @@ func (helperCommand) Synopsis() string {
 }
 
 func TestCommand_NoExist(t *testing.T) {
-	assert := asserts.NewTestingAsserts(t, true)
+	c := NewClient(&ClientConfig{Cmd: exec.Command("i-should-not-exist")})
+	defer c.Kill()
 
-	_, err := Command(exec.Command("i-should-never-ever-ever-exist"))
-	assert.NotNil(err, "should have an error")
+	_, err := c.Command()
+	if err == nil {
+		t.Fatal("should have error")
+	}
 }
 
 func TestCommand_Good(t *testing.T) {
-	assert := asserts.NewTestingAsserts(t, true)
-
-	command, err := Command(helperProcess("command"))
-	assert.Nil(err, "should start command properly")
-
-	assert.NotNil(command, "should have a command")
-	if command != nil {
-		result := command.Synopsis()
-		assert.Equal(result, "1", "should return result")
+	c := NewClient(&ClientConfig{Cmd: helperProcess("command")})
+	defer c.Kill()
 
-		result = command.Help()
-		assert.Equal(result, "2", "should return result")
+	command, err := c.Command()
+	if err != nil {
+		t.Fatalf("should not have error: %s", err)
 	}
-}
-
-func TestCommand_CommandExited(t *testing.T) {
-	assert := asserts.NewTestingAsserts(t, true)
 
-	_, err := Command(helperProcess("im-a-command-that-doesnt-work"))
-	assert.NotNil(err, "should have an error")
-	assert.Equal(err.Error(), "plugin exited before we could connect", "be correct error")
-}
-
-func TestCommand_BadRPC(t *testing.T) {
-	assert := asserts.NewTestingAsserts(t, true)
+	result := command.Synopsis()
+	if result != "1" {
+		t.Errorf("synopsis not correct: %s", result)
+	}
 
-	_, err := Command(helperProcess("invalid-rpc-address"))
-	assert.NotNil(err, "should have an error")
-	assert.Equal(err.Error(), "missing port in address lolinvalid", "be correct error")
+	result = command.Help()
+	if result != "2" {
+		t.Errorf("help not correct: %s", result)
+	}
 }

packer/plugin/hook.go

@@ -2,10 +2,7 @@ package plugin
 
 import (
 	"github.com/mitchellh/packer/packer"
-	packrpc "github.com/mitchellh/packer/packer/rpc"
 	"log"
-	"net/rpc"
-	"os/exec"
 )
 
 type cmdHook struct {
@@ -29,43 +26,3 @@ func (c *cmdHook) checkExit(p interface{}, cb func()) {
 		log.Panic(p)
 	}
 }
-
-// Returns a valid packer.Hook where the hook is executed via RPC
-// to a plugin that is within a subprocess.
-//
-// This method will start the given exec.Cmd, which should point to
-// the plugin binary to execute. Some configuration will be done to
-// the command, such as overriding Stdout and some environmental variables.
-//
-// This function guarantees the subprocess will end in a timely manner.
-func Hook(cmd *exec.Cmd) (result packer.Hook, err error) {
-	config := &ClientConfig{
-		Cmd: cmd,
-		Managed: true,
-	}
-
-	cmdClient := NewClient(config)
-	address, err := cmdClient.Start()
-	if err != nil {
-		return
-	}
-
-	defer func() {
-		// Make sure the command is properly killed in the case of an error
-		if err != nil {
-			cmdClient.Kill()
-		}
-	}()
-
-	client, err := rpc.Dial("tcp", address)
-	if err != nil {
-		return
-	}
-
-	result = &cmdHook{
-		packrpc.Hook(client),
-		cmdClient,
-	}
-
-	return
-}

packer/plugin/hook_test.go

@@ -1,7 +1,6 @@
 package plugin
 
 import (
-	"cgl.tideland.biz/asserts"
 	"github.com/mitchellh/packer/packer"
 	"os/exec"
 	"testing"
@@ -12,15 +11,21 @@ type helperHook byte
 func (helperHook) Run(string, packer.Ui, packer.Communicator, interface{}) {}
 
 func TestHook_NoExist(t *testing.T) {
-	assert := asserts.NewTestingAsserts(t, true)
+	c := NewClient(&ClientConfig{Cmd: exec.Command("i-should-not-exist")})
+	defer c.Kill()
 
-	_, err := Hook(exec.Command("i-should-never-ever-ever-exist"))
-	assert.NotNil(err, "should have an error")
+	_, err := c.Hook()
+	if err == nil {
+		t.Fatal("should have error")
+	}
 }
 
 func TestHook_Good(t *testing.T) {
-	assert := asserts.NewTestingAsserts(t, true)
+	c := NewClient(&ClientConfig{Cmd: helperProcess("hook")})
+	defer c.Kill()
 
-	_, err := Hook(helperProcess("hook"))
-	assert.Nil(err, "should start hook properly")
+	_, err := c.Hook()
+	if err != nil {
+		t.Fatalf("should not have error: %s", err)
+	}
 }

packer/plugin/provisioner.go

@@ -2,10 +2,7 @@ package plugin
 
 import (
 	"github.com/mitchellh/packer/packer"
-	packrpc "github.com/mitchellh/packer/packer/rpc"
 	"log"
-	"net/rpc"
-	"os/exec"
 )
 
 type cmdProvisioner struct {
@@ -38,43 +35,3 @@ func (c *cmdProvisioner) checkExit(p interface{}, cb func()) {
 		log.Panic(p)
 	}
 }
-
-// Returns a valid packer.Provisioner where the hook is executed via RPC
-// to a plugin that is within a subprocess.
-//
-// This method will start the given exec.Cmd, which should point to
-// the plugin binary to execute. Some configuration will be done to
-// the command, such as overriding Stdout and some environmental variables.
-//
-// This function guarantees the subprocess will end in a timely manner.
-func Provisioner(cmd *exec.Cmd) (result packer.Provisioner, err error) {
-	config := &ClientConfig{
-		Cmd: cmd,
-		Managed: true,
-	}
-
-	cmdClient := NewClient(config)
-	address, err := cmdClient.Start()
-	if err != nil {
-		return
-	}
-
-	defer func() {
-		// Make sure the command is properly killed in the case of an error
-		if err != nil {
-			cmdClient.Kill()
-		}
-	}()
-
-	client, err := rpc.Dial("tcp", address)
-	if err != nil {
-		return
-	}
-
-	result = &cmdProvisioner{
-		packrpc.Provisioner(client),
-		cmdClient,
-	}
-
-	return
-}

packer/plugin/provisioner_test.go

@@ -1,7 +1,6 @@
 package plugin
 
 import (
-	"cgl.tideland.biz/asserts"
 	"github.com/mitchellh/packer/packer"
 	"os/exec"
 	"testing"
@@ -16,15 +15,21 @@ func (helperProvisioner) Prepare(...interface{}) error {
 func (helperProvisioner) Provision(packer.Ui, packer.Communicator) {}
 
 func TestProvisioner_NoExist(t *testing.T) {
-	assert := asserts.NewTestingAsserts(t, true)
+	c := NewClient(&ClientConfig{Cmd: exec.Command("i-should-not-exist")})
+	defer c.Kill()
 
-	_, err := Provisioner(exec.Command("i-should-never-ever-ever-exist"))
-	assert.NotNil(err, "should have an error")
+	_, err := c.Provisioner()
+	if err == nil {
+		t.Fatal("should have error")
+	}
 }
 
 func TestProvisioner_Good(t *testing.T) {
-	assert := asserts.NewTestingAsserts(t, true)
+	c := NewClient(&ClientConfig{Cmd: helperProcess("provisioner")})
+	defer c.Kill()
 
-	_, err := Provisioner(helperProcess("provisioner"))
-	assert.Nil(err, "should start provisioner properly")
+	_, err := c.Provisioner()
+	if err != nil {
+		t.Fatalf("should not have error: %s", err)
+	}
 }