From 0c714592f0a73b3ae0dc9e3704cbfa146fa9cf2f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 28 Jul 2016 11:11:53 -0400 Subject: [PATCH] Fix variable handling on subsequent pushes The handling of remote variables was completely disabled for push. We still need to fetch variables from atlas for push, because if the variable is only set remotely the Input walk will still prompt the user for a value. We add the missing remote variables to the context to disable input. We now only handle remote variables as atlas.TFVar and explicitly pass around that type rather than an `interface{}`. Shorten the text fixture slightly to make the output a little more readable on failures. --- command/push.go | 98 +++++++++++++------- command/push_test.go | 105 ++++++++++++---------- command/test-fixtures/push-tfvars/main.tf | 3 +- 3 files changed, 121 insertions(+), 85 deletions(-) diff --git a/command/push.go b/command/push.go index 557887b7c..d7845ddb4 100644 --- a/command/push.go +++ b/command/push.go @@ -137,19 +137,28 @@ func (c *PushCommand) Run(args []string) int { c.client = &atlasPushClient{Client: client} } - // Get the variables we might already have + // Get the variables we already have in atlas atlasVars, err := c.client.Get(name) if err != nil { c.Ui.Error(fmt.Sprintf( "Error looking up previously pushed configuration: %s", err)) return 1 } - for k, v := range atlasVars { - if _, ok := overwriteMap[k]; ok { - continue - } - ctx.SetVariable(k, v) + // filter any overwrites from the atlas vars + for k := range overwriteMap { + delete(atlasVars, k) + } + + // Set remote variables in the context if we don't have a value here. These + // don't have to be correct, it just prevents the Input walk from prompting + // the user for input, The atlas variable may be an hcl-encoded object, but + // we're just going to set it as the raw string value. + ctxVars := ctx.Variables() + for k, av := range atlasVars { + if _, ok := ctxVars[k]; !ok { + ctx.SetVariable(k, av.Value) + } } // Ask for input @@ -159,6 +168,18 @@ func (c *PushCommand) Run(args []string) int { return 1 } + // Now that we've gone through the input walk, we can be sure we have all + // the variables we're going to get. + // We are going to keep these separate from the atlas variables until + // upload, so we can notify the user which local variables we're sending. + serializedVars, err := tfVars(ctx.Variables()) + if err != nil { + c.Ui.Error(fmt.Sprintf( + "An error has occurred while serializing the variables for uploading:\n"+ + "%s", err)) + return 1 + } + // Build the archiving options, which includes everything it can // by default according to VCS rules but forcing the data directory. archiveOpts := &archive.ArchiveOpts{ @@ -184,17 +205,23 @@ func (c *PushCommand) Run(args []string) int { // Output to the user the variables that will be uploaded var setVars []string - for k, _ := range ctx.Variables() { - if _, ok := overwriteMap[k]; !ok { - if _, ok := atlasVars[k]; ok { - // Atlas variable not within override, so it came from Atlas - continue - } + // variables to upload + var uploadVars []atlas.TFVar + + // Now we can combine the vars for upload to atlas and list the variables + // we're uploading for the user + for _, sv := range serializedVars { + if av, ok := atlasVars[sv.Key]; ok { + // this belongs to Atlas + uploadVars = append(uploadVars, av) + } else { + // we're uploading our local version + setVars = append(setVars, sv.Key) + uploadVars = append(uploadVars, sv) } - // This variable was set from the local value - setVars = append(setVars, k) } + sort.Strings(setVars) if len(setVars) > 0 { c.Ui.Output( @@ -210,21 +237,12 @@ func (c *PushCommand) Run(args []string) int { c.Ui.Output("") } - variables := ctx.Variables() - serializedVars, err := tfVars(variables) - if err != nil { - c.Ui.Error(fmt.Sprintf( - "An error has occurred while serializing the variables for uploading:\n"+ - "%s", err)) - return 1 - } - // Upsert! opts := &pushUpsertOptions{ Name: name, Archive: archiveR, Variables: ctx.Variables(), - TFVars: serializedVars, + TFVars: uploadVars, } c.Ui.Output("Uploading Terraform configuration...") @@ -340,10 +358,11 @@ func (c *PushCommand) Synopsis() string { return "Upload this Terraform module to Atlas to run" } -// pushClient is implementd internally to control where pushes go. This is -// either to Atlas or a mock for testing. +// pushClient is implemented internally to control where pushes go. This is +// either to Atlas or a mock for testing. We still return a map to make it +// easier to check for variable existence when filtering the overrides. type pushClient interface { - Get(string) (map[string]interface{}, error) + Get(string) (map[string]atlas.TFVar, error) Upsert(*pushUpsertOptions) (int, error) } @@ -358,7 +377,7 @@ type atlasPushClient struct { Client *atlas.Client } -func (c *atlasPushClient) Get(name string) (map[string]interface{}, error) { +func (c *atlasPushClient) Get(name string) (map[string]atlas.TFVar, error) { user, name, err := atlas.ParseSlug(name) if err != nil { return nil, err @@ -369,10 +388,21 @@ func (c *atlasPushClient) Get(name string) (map[string]interface{}, error) { return nil, err } - var variables map[string]interface{} - if version != nil { - // TODO: merge variables and TFVars - //variables = version.Variables + variables := make(map[string]atlas.TFVar) + + if version == nil { + return variables, nil + } + + // Variables is superseded by TFVars + if version.TFVars == nil { + for k, v := range version.Variables { + variables[k] = atlas.TFVar{Key: k, Value: v} + } + } else { + for _, v := range version.TFVars { + variables[v.Key] = v + } } return variables, nil @@ -402,7 +432,7 @@ type mockPushClient struct { GetCalled bool GetName string - GetResult map[string]interface{} + GetResult map[string]atlas.TFVar GetError error UpsertCalled bool @@ -411,7 +441,7 @@ type mockPushClient struct { UpsertError error } -func (c *mockPushClient) Get(name string) (map[string]interface{}, error) { +func (c *mockPushClient) Get(name string) (map[string]atlas.TFVar, error) { c.GetCalled = true c.GetName = name return c.GetResult, c.GetError diff --git a/command/push_test.go b/command/push_test.go index 3db6f7739..60270169e 100644 --- a/command/push_test.go +++ b/command/push_test.go @@ -119,11 +119,13 @@ func TestPush_input(t *testing.T) { variables := map[string]interface{}{ "foo": "foo", } + if !reflect.DeepEqual(client.UpsertOptions.Variables, variables) { t.Fatalf("bad: %#v", client.UpsertOptions.Variables) } } +// We want a variable from atlas to fill a missing variable locally func TestPush_inputPartial(t *testing.T) { tmp, cwd := testCwd(t) defer testFixCwd(t, tmp, cwd) @@ -143,8 +145,10 @@ func TestPush_inputPartial(t *testing.T) { defer os.Remove(archivePath) client := &mockPushClient{ - File: archivePath, - GetResult: map[string]interface{}{"foo": "bar"}, + File: archivePath, + GetResult: map[string]atlas.TFVar{ + "foo": atlas.TFVar{Key: "foo", Value: "bar"}, + }, } ui := new(cli.MockUi) c := &PushCommand{ @@ -171,12 +175,13 @@ func TestPush_inputPartial(t *testing.T) { t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) } - variables := map[string]interface{}{ - "foo": "bar", - "bar": "foo", + expectedTFVars := []atlas.TFVar{ + {Key: "bar", Value: "foo"}, + {Key: "foo", Value: "bar"}, } - if !reflect.DeepEqual(client.UpsertOptions.Variables, variables) { - t.Fatalf("bad: %#v", client.UpsertOptions) + if !reflect.DeepEqual(client.UpsertOptions.TFVars, expectedTFVars) { + t.Logf("expected: %#v", expectedTFVars) + t.Fatalf("got: %#v", client.UpsertOptions.TFVars) } } @@ -209,8 +214,11 @@ func TestPush_localOverride(t *testing.T) { client := &mockPushClient{File: archivePath} // Provided vars should override existing ones - client.GetResult = map[string]interface{}{ - "foo": "old", + client.GetResult = map[string]atlas.TFVar{ + "foo": atlas.TFVar{ + Key: "foo", + Value: "old", + }, } ui := new(cli.MockUi) c := &PushCommand{ @@ -248,10 +256,11 @@ func TestPush_localOverride(t *testing.T) { t.Fatalf("bad: %#v", client.UpsertOptions) } - variables := pushTFVars() + expectedTFVars := pushTFVars() - if !reflect.DeepEqual(client.UpsertOptions.Variables, variables) { - t.Fatalf("bad: %#v", client.UpsertOptions) + if !reflect.DeepEqual(client.UpsertOptions.TFVars, expectedTFVars) { + t.Logf("expected: %#v", expectedTFVars) + t.Fatalf("got: %#v", client.UpsertOptions.TFVars) } } @@ -284,8 +293,11 @@ func TestPush_preferAtlas(t *testing.T) { client := &mockPushClient{File: archivePath} // Provided vars should override existing ones - client.GetResult = map[string]interface{}{ - "foo": "old", + client.GetResult = map[string]atlas.TFVar{ + "foo": atlas.TFVar{ + Key: "foo", + Value: "old", + }, } ui := new(cli.MockUi) c := &PushCommand{ @@ -322,11 +334,17 @@ func TestPush_preferAtlas(t *testing.T) { t.Fatalf("bad: %#v", client.UpsertOptions) } - variables := pushTFVars() - variables["foo"] = "old" + // change the expected response to match our change + expectedTFVars := pushTFVars() + for i, v := range expectedTFVars { + if v.Key == "foo" { + expectedTFVars[i] = atlas.TFVar{Key: "foo", Value: "old"} + } + } - if !reflect.DeepEqual(client.UpsertOptions.Variables, variables) { - t.Fatalf("bad: %#v", client.UpsertOptions.Variables) + if !reflect.DeepEqual(expectedTFVars, client.UpsertOptions.TFVars) { + t.Logf("expected: %#v", expectedTFVars) + t.Fatalf("got: %#v", client.UpsertOptions.TFVars) } } @@ -392,27 +410,8 @@ func TestPush_tfvars(t *testing.T) { t.Fatalf("bad: %#v", client.UpsertOptions) } - variables := pushTFVars() - - // make sure these dind't go missing for some reason - for k, v := range variables { - if !reflect.DeepEqual(client.UpsertOptions.Variables[k], v) { - t.Fatalf("bad: %#v", client.UpsertOptions.Variables[k]) - } - } - //now check TFVars - tfvars := []atlas.TFVar{ - {"bar", "foo", false}, - {"baz", `{ - A = "a" - B = "b" - interp = "${file("t.txt")}" -} -`, true}, - {"fob", `["a", "b", "c", "quotes \"in\" quotes"]` + "\n", true}, - {"foo", "bar", false}, - } + tfvars := pushTFVars() for i, expected := range tfvars { got := client.UpsertOptions.TFVars[i] @@ -584,16 +583,24 @@ func testArchiveStr(t *testing.T, path string) []string { return result } -// the structure returned from the push-tfvars test fixture -func pushTFVars() map[string]interface{} { - return map[string]interface{}{ - "foo": "bar", - "bar": "foo", - "baz": map[string]interface{}{ - "A": "a", - "B": "b", - "interp": `${file("t.txt")}`, - }, - "fob": []interface{}{"a", "b", "c", `quotes "in" quotes`}, +func pushTFVars() []atlas.TFVar { + return []atlas.TFVar{ + {"bar", "foo", false}, + {"baz", `{ + A = "a" + interp = "${file("t.txt")}" +} +`, true}, + {"fob", `["a", "quotes \"in\" quotes"]` + "\n", true}, + {"foo", "bar", false}, } } + +// the structure returned from the push-tfvars test fixture +func pushTFVarsMap() map[string]atlas.TFVar { + vars := make(map[string]atlas.TFVar) + for _, v := range pushTFVars() { + vars[v.Key] = v + } + return vars +} diff --git a/command/test-fixtures/push-tfvars/main.tf b/command/test-fixtures/push-tfvars/main.tf index 528b6ed60..2110bea73 100644 --- a/command/test-fixtures/push-tfvars/main.tf +++ b/command/test-fixtures/push-tfvars/main.tf @@ -7,14 +7,13 @@ variable "baz" { default = { "A" = "a" - "B" = "b" interp = "${file("t.txt")}" } } variable "fob" { type = "list" - default = ["a", "b", "c", "quotes \"in\" quotes"] + default = ["a", "quotes \"in\" quotes"] } resource "test_instance" "foo" {}