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

@@ -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

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
 }
 
+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 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 pushTFVarsMap() map[string]atlas.TFVar {
+	vars := make(map[string]atlas.TFVar)
+	for _, v := range pushTFVars() {
+		vars[v.Key] = v
 	}
+	return vars
 }

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" {}