From 7cb31714ad8868e7e97ec4714bc1e973c6537a5e Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Thu, 7 Mar 2019 09:37:31 -0800 Subject: [PATCH 1/5] Allow user variables to be interpreted within the variables section of the template. --- packer/core.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packer/core.go b/packer/core.go index d88e21e5c..cd6c6ae34 100644 --- a/packer/core.go +++ b/packer/core.go @@ -303,7 +303,7 @@ func (c *Core) init() error { // Go through the variables and interpolate the environment variables ctx := c.Context() ctx.EnableEnv = true - ctx.UserVariables = nil + ctx.UserVariables = make(map[string]string) for k, v := range c.Template.Variables { // Ignore variables that are required if v.Required { @@ -324,6 +324,7 @@ func (c *Core) init() error { } c.variables[k] = def + ctx.UserVariables = c.variables } for _, v := range c.Template.SensitiveVariables { From bf0d7b36203bb4f4bc869d2d3c22e6044288c1c4 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Fri, 8 Mar 2019 14:49:47 -0800 Subject: [PATCH 2/5] make it work and add tests --- packer/core.go | 88 ++++++++++++++++++++++++++++------- packer/core_test.go | 52 +++++++++++++++++++++ template/interpolate/funcs.go | 10 +++- 3 files changed, 131 insertions(+), 19 deletions(-) diff --git a/packer/core.go b/packer/core.go index cd6c6ae34..630a7dbd5 100644 --- a/packer/core.go +++ b/packer/core.go @@ -3,6 +3,7 @@ package packer import ( "fmt" "sort" + "strings" multierror "github.com/hashicorp/go-multierror" version "github.com/hashicorp/go-version" @@ -300,31 +301,82 @@ func (c *Core) init() error { c.variables = make(map[string]string) } - // Go through the variables and interpolate the environment variables + // Go through the variables and interpolate the environment and + // user variables + ctx := c.Context() ctx.EnableEnv = true ctx.UserVariables = make(map[string]string) - for k, v := range c.Template.Variables { - // Ignore variables that are required - if v.Required { - continue - } + shouldRetry := true + tryCount := 0 + changed := false + failedInterpolation := "" + + // Why this giant loop? User variables can be recursively defined. For + // example: + // "variables": { + // "foo": "bar", + // "baz": "{{user `foo`}}baz", + // "bang": "bang{{user `baz`}}" + // }, + // In this situation, we cannot guarantee that we've added "foo" to + // UserVariables before we try to interpolate "baz" the first time. We need + // to have the option to loop back over in order to add the properly + // interpolated "baz" to the UserVariables map. + // Likewise, we'd need to loop up to two times to properly add "bang", + // since that depends on "baz" being set, which depends on "foo" being set. + + // We break out of the while loop either if all our variables have been + // interpolated or if after 100 loops we still haven't succeeded in + // interpolating them. Please don't actually nest your variables in 100 + // layers of other variables. Please. + + for shouldRetry == true { + shouldRetry = false + for k, v := range c.Template.Variables { + // Ignore variables that are required + if v.Required { + continue + } - // Ignore variables that have a value - if _, ok := c.variables[k]; ok { - continue - } + // Ignore variables that have a value already + if _, ok := c.variables[k]; ok { + continue + } - // Interpolate the default - def, err := interpolate.Render(v.Default, ctx) - if err != nil { - return fmt.Errorf( - "error interpolating default value for '%s': %s", - k, err) + // Interpolate the default + def, err := interpolate.Render(v.Default, ctx) + if err != nil { + if strings.Contains(err.Error(), "error calling user") { + shouldRetry = true + tryCount++ + failedInterpolation = fmt.Sprintf(`"%s": "%s"`, k, v.Default) + continue + } else { + return fmt.Errorf( + // unexpected interpolation error: abort the run + "error interpolating default value for '%s': %s", + k, err) + } + } + + // We only get here if interpolation has succeeded, so something is + // different in this loop than in the last one. + changed = true + c.variables[k] = def + ctx.UserVariables = c.variables + } + if tryCount >= 100 { + break } - c.variables[k] = def - ctx.UserVariables = c.variables + } + + if (changed == false) && (shouldRetry == true) { + return fmt.Errorf("Failed to interpolate %s: Please make sure that "+ + "the variable you're referencing has been defined; Packer treats "+ + "all variables used to interpolate other user varaibles as "+ + "required.", failedInterpolation) } for _, v := range c.Template.SensitiveVariables { diff --git a/packer/core_test.go b/packer/core_test.go index fb3774d7b..d3f2b6ed9 100644 --- a/packer/core_test.go +++ b/packer/core_test.go @@ -523,6 +523,58 @@ func TestCoreValidate(t *testing.T) { } } +func TestCore_InterpolateUserVars(t *testing.T) { + cases := []struct { + File string + Expected map[string]string + Err bool + }{ + { + "variables.json", + map[string]string{ + "foo": "bar", + "bar": "bar", + "baz": "barbaz", + "bang": "bangbarbaz", + }, + false, + }, + { + "variables2.json", + map[string]string{}, + true, + }, + } + for _, tc := range cases { + f, err := os.Open(fixtureDir(tc.File)) + if err != nil { + t.Fatalf("err: %s", err) + } + + tpl, err := template.Parse(f) + f.Close() + if err != nil { + t.Fatalf("err: %s\n\n%s", tc.File, err) + } + + ccf, err := NewCore(&CoreConfig{ + Template: tpl, + Version: "1.0.0", + }) + + if (err != nil) != tc.Err { + t.Fatalf("err: %s\n\n%s", tc.File, err) + } + if !tc.Err { + for k, v := range ccf.variables { + if tc.Expected[k] != v { + t.Fatalf("Expected %s but got %s", tc.Expected[k], v) + } + } + } + } +} + func TestSensitiveVars(t *testing.T) { cases := []struct { File string diff --git a/template/interpolate/funcs.go b/template/interpolate/funcs.go index 18c9b7407..57495052a 100644 --- a/template/interpolate/funcs.go +++ b/template/interpolate/funcs.go @@ -163,7 +163,15 @@ func funcGenUser(ctx *Context) interface{} { return "", errors.New("test") } - return ctx.UserVariables[k], nil + val, ok := ctx.UserVariables[k] + if ctx.EnableEnv { + // error and retry if we're interpolating UserVariables. But if + // we're elsewhere in the template, just return the empty string. + if !ok { + return "", errors.New(fmt.Sprintf("variable %s not set", k)) + } + } + return val, nil } } From a62db1a0c62d9caf45f9a8577fda8ef589722a12 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Fri, 8 Mar 2019 14:50:05 -0800 Subject: [PATCH 3/5] add tests fixtures --- packer/test-fixtures/variables.json | 11 +++++++++++ packer/test-fixtures/variables2.json | 10 ++++++++++ 2 files changed, 21 insertions(+) create mode 100644 packer/test-fixtures/variables.json create mode 100644 packer/test-fixtures/variables2.json diff --git a/packer/test-fixtures/variables.json b/packer/test-fixtures/variables.json new file mode 100644 index 000000000..b3b32a7bd --- /dev/null +++ b/packer/test-fixtures/variables.json @@ -0,0 +1,11 @@ +{ + "variables": { + "foo": "bar", + "bar": "{{user `foo`}}", + "baz": "{{user `bar`}}baz", + "bang": "bang{{user `baz`}}" + }, + "builders": [ + {"type": "foo"} + ] +} diff --git a/packer/test-fixtures/variables2.json b/packer/test-fixtures/variables2.json new file mode 100644 index 000000000..3d4019756 --- /dev/null +++ b/packer/test-fixtures/variables2.json @@ -0,0 +1,10 @@ +{ + "variables": { + "bar": "{{user `foo`}}", + "baz": "{{user `bar`}}baz", + "bang": "bang{{user `baz`}}" + }, + "builders": [ + {"type": "foo"} + ] +} From 86c3c44afec3b6795f25fac46466828b8d4fb441 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 13 Mar 2019 14:59:05 -0700 Subject: [PATCH 4/5] switch on err type not string --- packer/core.go | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/packer/core.go b/packer/core.go index 630a7dbd5..b9fc2ff9b 100644 --- a/packer/core.go +++ b/packer/core.go @@ -3,7 +3,8 @@ package packer import ( "fmt" "sort" - "strings" + + ttmp "text/template" multierror "github.com/hashicorp/go-multierror" version "github.com/hashicorp/go-version" @@ -346,25 +347,29 @@ func (c *Core) init() error { // Interpolate the default def, err := interpolate.Render(v.Default, ctx) - if err != nil { - if strings.Contains(err.Error(), "error calling user") { - shouldRetry = true - tryCount++ - failedInterpolation = fmt.Sprintf(`"%s": "%s"`, k, v.Default) - continue - } else { - return fmt.Errorf( - // unexpected interpolation error: abort the run - "error interpolating default value for '%s': %s", - k, err) - } - } + switch err.(type) { + case nil: + // We only get here if interpolation has succeeded, so something is + // different in this loop than in the last one. + changed = true + c.variables[k] = def + ctx.UserVariables = c.variables + case ttmp.ExecError: + shouldRetry = true + tryCount++ + failedInterpolation = fmt.Sprintf(`"%s": "%s"`, k, v.Default) + continue - // We only get here if interpolation has succeeded, so something is - // different in this loop than in the last one. - changed = true - c.variables[k] = def - ctx.UserVariables = c.variables + return fmt.Errorf( + // unexpected interpolation error: abort the run + "error interpolating default value for '%s': %s", + k, err) + default: + return fmt.Errorf( + // unexpected interpolation error: abort the run + "error interpolating default value for '%s': %s", + k, err) + } } if tryCount >= 100 { break From b52ee147766e7987e872046f52d26a048febe867 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Thu, 14 Mar 2019 10:05:10 -0700 Subject: [PATCH 5/5] remove unreachable code --- packer/core.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packer/core.go b/packer/core.go index b9fc2ff9b..a76951e33 100644 --- a/packer/core.go +++ b/packer/core.go @@ -359,11 +359,6 @@ func (c *Core) init() error { tryCount++ failedInterpolation = fmt.Sprintf(`"%s": "%s"`, k, v.Default) continue - - return fmt.Errorf( - // unexpected interpolation error: abort the run - "error interpolating default value for '%s': %s", - k, err) default: return fmt.Errorf( // unexpected interpolation error: abort the run