From 7fe98e50fe2ce3169b38ee5b2ca4b2e8a5386b98 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 11 Jun 2013 11:00:06 -0700 Subject: [PATCH] packer/plugin: Refactor to get interfaces from Client --- packer/plugin/builder.go | 43 ----------------- packer/plugin/builder_test.go | 19 +++++--- packer/plugin/client.go | 77 +++++++++++++++++++++++++++---- packer/plugin/client_test.go | 16 ------- packer/plugin/command.go | 43 ----------------- packer/plugin/command_test.go | 54 +++++++++------------- packer/plugin/hook.go | 43 ----------------- packer/plugin/hook_test.go | 19 +++++--- packer/plugin/provisioner.go | 43 ----------------- packer/plugin/provisioner_test.go | 19 +++++--- 10 files changed, 127 insertions(+), 249 deletions(-) diff --git a/packer/plugin/builder.go b/packer/plugin/builder.go index e717116ab..00da4c99b 100644 --- a/packer/plugin/builder.go +++ b/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 -} diff --git a/packer/plugin/builder_test.go b/packer/plugin/builder_test.go index b149c18c4..bb8e7b00b 100644 --- a/packer/plugin/builder_test.go +++ b/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) + } } diff --git a/packer/plugin/client.go b/packer/plugin/client.go index c8e82d559..48cc2060d 100644 --- a/packer/plugin/client.go +++ b/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 +} diff --git a/packer/plugin/client_test.go b/packer/plugin/client_test.go index f8c4ec75e..5b757c206 100644 --- a/packer/plugin/client_test.go +++ b/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"), diff --git a/packer/plugin/command.go b/packer/plugin/command.go index 6d27066be..13c540ed7 100644 --- a/packer/plugin/command.go +++ b/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 -} diff --git a/packer/plugin/command_test.go b/packer/plugin/command_test.go index 3e6816ade..5c860c89c 100644 --- a/packer/plugin/command_test.go +++ b/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") -} - -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") - - result = command.Help() - assert.Equal(result, "2", "should return result") + _, err := c.Command() + if err == nil { + t.Fatal("should have error") } } -func TestCommand_CommandExited(t *testing.T) { - assert := asserts.NewTestingAsserts(t, true) +func TestCommand_Good(t *testing.T) { + c := NewClient(&ClientConfig{Cmd: helperProcess("command")}) + defer c.Kill() - _, 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) - - _, 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") + command, err := c.Command() + if err != nil { + t.Fatalf("should not have error: %s", err) + } + + result := command.Synopsis() + if result != "1" { + t.Errorf("synopsis not correct: %s", result) + } + + result = command.Help() + if result != "2" { + t.Errorf("help not correct: %s", result) + } } diff --git a/packer/plugin/hook.go b/packer/plugin/hook.go index afb0968ed..c90d4645a 100644 --- a/packer/plugin/hook.go +++ b/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 -} diff --git a/packer/plugin/hook_test.go b/packer/plugin/hook_test.go index 8680be903..f705853df 100644 --- a/packer/plugin/hook_test.go +++ b/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) + } } diff --git a/packer/plugin/provisioner.go b/packer/plugin/provisioner.go index 2ef425e5f..92ad848a4 100644 --- a/packer/plugin/provisioner.go +++ b/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 -} diff --git a/packer/plugin/provisioner_test.go b/packer/plugin/provisioner_test.go index eaf08822a..2917cd55d 100644 --- a/packer/plugin/provisioner_test.go +++ b/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) + } }