From 869732826bce163ac71cb260a4a29b6aabeaed8c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 7 May 2013 20:42:49 -0700 Subject: [PATCH] packer, packer/rpc: Make command/builderFunc support errors --- packer/environment.go | 49 +++++++++++++++++++++++----------- packer/environment_test.go | 43 ++++++++++++++++++----------- packer/rpc/environment.go | 32 ++++++++++++++-------- packer/rpc/environment_test.go | 12 ++++----- packer/template.go | 8 ++++-- packer/template_test.go | 4 +-- 6 files changed, 96 insertions(+), 52 deletions(-) diff --git a/packer/environment.go b/packer/environment.go index b3e6a6a6a..a6f9694d0 100644 --- a/packer/environment.go +++ b/packer/environment.go @@ -9,9 +9,9 @@ import ( "strings" ) -type BuilderFunc func(name string) Builder +type BuilderFunc func(name string) (Builder, error) -type CommandFunc func(name string) Command +type CommandFunc func(name string) (Command, error) // The environment interface provides access to the configuration and // state of a single Packer run. @@ -19,8 +19,8 @@ type CommandFunc func(name string) Command // It allows for things such as executing CLI commands, getting the // list of available builders, and more. type Environment interface { - Builder(name string) Builder - Cli(args []string) int + Builder(name string) (Builder, error) + Cli(args []string) (int, error) Ui() Ui } @@ -45,8 +45,8 @@ type EnvironmentConfig struct { // be used to create a new enviroment with NewEnvironment with sane defaults. func DefaultEnvironmentConfig() *EnvironmentConfig { config := &EnvironmentConfig{} - config.BuilderFunc = func(string) Builder { return nil } - config.CommandFunc = func(string) Command { return nil } + config.BuilderFunc = func(string) (Builder, error) { return nil, nil } + config.CommandFunc = func(string) (Command, error) { return nil, nil } config.Commands = make([]string, 0) config.Ui = &ReaderWriterUi{os.Stdin, os.Stdout} return config @@ -71,16 +71,25 @@ func NewEnvironment(config *EnvironmentConfig) (resultEnv Environment, err error // Returns a builder of the given name that is registered with this // environment. -func (e *coreEnvironment) Builder(name string) Builder { - return e.builderFunc(name) +func (e *coreEnvironment) Builder(name string) (b Builder, err error) { + b, err = e.builderFunc(name) + if err != nil { + return + } + + if b == nil { + err = fmt.Errorf("No builder returned for name: %s", name) + } + + return } // Executes a command as if it was typed on the command-line interface. // The return value is the exit code of the command. -func (e *coreEnvironment) Cli(args []string) int { +func (e *coreEnvironment) Cli(args []string) (result int, err error) { if len(args) == 0 || args[0] == "--help" || args[0] == "-h" { e.printHelp() - return 1 + return 1, nil } version := args[0] == "version" @@ -99,16 +108,19 @@ func (e *coreEnvironment) Cli(args []string) int { } if command == nil { - command = e.commandFunc(args[0]) + command, err = e.commandFunc(args[0]) + if err != nil { + return + } // If we still don't have a command, show the help. if command == nil { e.printHelp() - return 1 + return 1, nil } } - return command.Run(e, args[1:]) + return command.Run(e, args[1:]), nil } // Prints the CLI help to the UI. @@ -131,13 +143,20 @@ func (e *coreEnvironment) printHelp() { e.ui.Say("usage: packer [--version] [--help] []\n\n") e.ui.Say("Available commands are:\n") for _, key := range e.commands { - command := e.commandFunc(key) + var synopsis string + + command, err := e.commandFunc(key) + if err != nil { + synopsis = fmt.Sprintf("Error loading command: %s", err.Error()) + } else { + synopsis = command.Synopsis() + } // Pad the key with spaces so that they're all the same width key = fmt.Sprintf("%v%v", key, strings.Repeat(" ", maxKeyLen-len(key))) // Output the command and the synopsis - e.ui.Say(" %v %v\n", key, command.Synopsis()) + e.ui.Say(" %v %v\n", key, synopsis) } } diff --git a/packer/environment_test.go b/packer/environment_test.go index 3d570d820..6265b825d 100644 --- a/packer/environment_test.go +++ b/packer/environment_test.go @@ -3,6 +3,7 @@ package packer import ( "bytes" "cgl.tideland.biz/asserts" + "fmt" "os" "strings" "testing" @@ -58,10 +59,12 @@ func TestEnvironment_Builder(t *testing.T) { builders["foo"] = builder config := DefaultEnvironmentConfig() - config.BuilderFunc = func(n string) Builder { return builders[n] } + config.BuilderFunc = func(n string) (Builder, error) { return builders[n], nil } env, _ := NewEnvironment(config) - assert.Equal(env.Builder("foo"), builder, "should return correct builder") + returnedBuilder, err := env.Builder("foo") + assert.Nil(err, "should be no error") + assert.Equal(returnedBuilder, builder, "should return correct builder") } func TestEnvironment_Cli_CallsRun(t *testing.T) { @@ -73,10 +76,12 @@ func TestEnvironment_Cli_CallsRun(t *testing.T) { config := &EnvironmentConfig{} config.Commands = []string{"foo"} - config.CommandFunc = func(n string) Command { return commands[n] } + config.CommandFunc = func(n string) (Command, error) { return commands[n], nil } env, _ := NewEnvironment(config) - assert.Equal(env.Cli([]string{"foo", "bar", "baz"}), 0, "runs foo command") + exitCode, err := env.Cli([]string{"foo", "bar", "baz"}) + assert.Nil(err, "should be no error") + assert.Equal(exitCode, 0, "runs foo command") assert.True(command.runCalled, "run should've been called") assert.Equal(command.runEnv, env, "should've ran with env") assert.Equal(command.runArgs, []string{"bar", "baz"}, "should have right args") @@ -87,7 +92,8 @@ func TestEnvironment_DefaultCli_Empty(t *testing.T) { defaultEnv := testEnvironment() - assert.Equal(defaultEnv.Cli([]string{}), 1, "CLI with no args") + exitCode, _ := defaultEnv.Cli([]string{}) + assert.Equal(exitCode, 1, "CLI with no args") } func TestEnvironment_DefaultCli_Help(t *testing.T) { @@ -104,11 +110,13 @@ func TestEnvironment_DefaultCli_Help(t *testing.T) { } // Test "--help" - assert.Equal(defaultEnv.Cli([]string{"--help"}), 1, "--help should print") + exitCode, _ := defaultEnv.Cli([]string{"--help"}) + assert.Equal(exitCode, 1, "--help should print") testOutput() // Test "-h" - assert.Equal(defaultEnv.Cli([]string{"-h"}), 1, "--help should print") + exitCode, _ = defaultEnv.Cli([]string{"--help"}) + assert.Equal(exitCode, 1, "--help should print") testOutput() } @@ -117,17 +125,20 @@ func TestEnvironment_DefaultCli_Version(t *testing.T) { defaultEnv := testEnvironment() - // Test the basic version options - assert.Equal(defaultEnv.Cli([]string{"version"}), 0, "version should work") - assert.Equal(defaultEnv.Cli([]string{"--version"}), 0, "--version should work") - assert.Equal(defaultEnv.Cli([]string{"-v"}), 0, "-v should work") + versionCommands := []string{"version", "--version", "-v"} + for _, command := range versionCommands { + exitCode, _ := defaultEnv.Cli([]string{command}) + assert.Equal(exitCode, 0, fmt.Sprintf("%s should work", command)) - // Test the --version and -v can appear anywhere - assert.Equal(defaultEnv.Cli([]string{"bad", "-v"}), 0, "-v should work anywhere") - assert.Equal(defaultEnv.Cli([]string{"bad", "--version"}), 0, "--version should work anywhere") + // Test the --version and -v can appear anywhere + exitCode, _ = defaultEnv.Cli([]string{"bad", command}) - // Test that "version" can't appear anywhere - assert.Equal(defaultEnv.Cli([]string{"bad", "version"}), 1, "version should NOT work anywhere") + if command != "version" { + assert.Equal(exitCode, 0, fmt.Sprintf("%s should work anywhere", command)) + } else { + assert.Equal(exitCode, 1, fmt.Sprintf("%s should NOT work anywhere", command)) + } + } } func TestEnvironment_SettingUi(t *testing.T) { diff --git a/packer/rpc/environment.go b/packer/rpc/environment.go index d129373c8..f62a771f1 100644 --- a/packer/rpc/environment.go +++ b/packer/rpc/environment.go @@ -21,18 +21,25 @@ type EnvironmentCliArgs struct { Args []string } -func (e *Environment) Builder(name string) packer.Builder { +func (e *Environment) Builder(name string) (b packer.Builder, err error) { var reply string - e.client.Call("Environment.Builder", name, &reply) + err = e.client.Call("Environment.Builder", name, &reply) + if err != nil { + return + } - // TODO: error handling - client, _ := rpc.Dial("tcp", reply) - return &Builder{client} + client, err := rpc.Dial("tcp", reply) + if err != nil { + return + } + + b = &Builder{client} + return } -func (e *Environment) Cli(args []string) (result int) { +func (e *Environment) Cli(args []string) (result int, err error) { rpcArgs := &EnvironmentCliArgs{args} - e.client.Call("Environment.Cli", rpcArgs, &result) + err = e.client.Call("Environment.Cli", rpcArgs, &result) return } @@ -46,7 +53,10 @@ func (e *Environment) Ui() packer.Ui { } func (e *EnvironmentServer) Builder(name *string, reply *string) error { - builder := e.env.Builder(*name) + builder, err := e.env.Builder(*name) + if err != nil { + return err + } // Wrap it server := rpc.NewServer() @@ -56,9 +66,9 @@ func (e *EnvironmentServer) Builder(name *string, reply *string) error { return nil } -func (e *EnvironmentServer) Cli(args *EnvironmentCliArgs, reply *int) error { - *reply = e.env.Cli(args.Args) - return nil +func (e *EnvironmentServer) Cli(args *EnvironmentCliArgs, reply *int) (err error) { + *reply, err = e.env.Cli(args.Args) + return } func (e *EnvironmentServer) Ui(args *interface{}, reply *string) error { diff --git a/packer/rpc/environment_test.go b/packer/rpc/environment_test.go index 1d59a4ab4..33810d6fe 100644 --- a/packer/rpc/environment_test.go +++ b/packer/rpc/environment_test.go @@ -18,16 +18,16 @@ type testEnvironment struct { uiCalled bool } -func (e *testEnvironment) Builder(name string) packer.Builder { +func (e *testEnvironment) Builder(name string) (packer.Builder, error) { e.builderCalled = true e.builderName = name - return testEnvBuilder + return testEnvBuilder, nil } -func (e *testEnvironment) Cli(args []string) int { +func (e *testEnvironment) Cli(args []string) (int, error) { e.cliCalled = true e.cliArgs = args - return 42 + return 42, nil } func (e *testEnvironment) Ui() packer.Ui { @@ -52,7 +52,7 @@ func TestEnvironmentRPC(t *testing.T) { eClient := &Environment{client} // Test Builder - builder := eClient.Builder("foo") + builder, _ := eClient.Builder("foo") assert.True(e.builderCalled, "Builder should be called") assert.Equal(e.builderName, "foo", "Correct name for Builder") @@ -61,7 +61,7 @@ func TestEnvironmentRPC(t *testing.T) { // Test Cli cliArgs := []string{"foo", "bar"} - result := eClient.Cli(cliArgs) + result, _ := eClient.Cli(cliArgs) assert.True(e.cliCalled, "CLI should be called") assert.Equal(e.cliArgs, cliArgs, "args should match") assert.Equal(result, 42, "result shuld be 42") diff --git a/packer/template.go b/packer/template.go index 7877d502e..a0fb2b4c8 100644 --- a/packer/template.go +++ b/packer/template.go @@ -102,9 +102,13 @@ func (t *Template) Build(name string, bf BuilderFunc) (b Build, err error) { return } - builder := bf(builderConfig.builderName) + builder, err := bf(builderConfig.builderName) + if err != nil { + return + } + if builder == nil { - err = fmt.Errorf("Builder could not be found: %s", builderConfig.builderName) + err = fmt.Errorf("Builder not found: %s", name) return } diff --git a/packer/template_test.go b/packer/template_test.go index 3f19044d2..c7ac02686 100644 --- a/packer/template_test.go +++ b/packer/template_test.go @@ -157,7 +157,7 @@ func TestTemplate_BuildUnknownBuilder(t *testing.T) { template, err := ParseTemplate([]byte(data)) assert.Nil(err, "should not error") - builderFactory := func(string) Builder { return nil } + builderFactory := func(string) (Builder, error) { return nil, nil } build, err := template.Build("test1", builderFactory) assert.Nil(build, "build should be nil") assert.NotNil(err, "should have error") @@ -191,7 +191,7 @@ func TestTemplate_Build(t *testing.T) { "test-builder": builder, } - builderFactory := func(n string) Builder { return builderMap[n] } + builderFactory := func(n string) (Builder, error) { return builderMap[n], nil } // Get the build, verifying we can get it without issue, but also // that the proper builder was looked up and used for the build.