Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type check variables between modules #6185

Merged
merged 3 commits into from
Apr 15, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,17 @@ const (
VariableTypeMap
)

func (v VariableType) Printable() string {
switch v {
case VariableTypeString:
return "string"
case VariableTypeMap:
return "map"
default:
return "unknown"
}
}

// ProviderConfigName returns the name of the provider configuration in
// the given mapping that maps to the proper provider configuration
// for this resource.
Expand Down
35 changes: 35 additions & 0 deletions terraform/context_plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,40 @@ func TestContext2Plan_moduleVar(t *testing.T) {
}
}

func TestContext2Plan_moduleVarWrongType(t *testing.T) {
m := testModule(t, "plan-module-wrong-var-type")
p := testProvider("aws")
p.DiffFn = testDiffFn
ctx := testContext2(t, &ContextOpts{
Module: m,
Providers: map[string]ResourceProviderFactory{
"aws": testProviderFuncFixed(p),
},
})

_, err := ctx.Plan()
if err == nil {
t.Fatalf("should error")
}
}

func TestContext2Plan_moduleVarWrongTypeNested(t *testing.T) {
m := testModule(t, "plan-module-wrong-var-type-nested")
p := testProvider("aws")
p.DiffFn = testDiffFn
ctx := testContext2(t, &ContextOpts{
Module: m,
Providers: map[string]ResourceProviderFactory{
"aws": testProviderFuncFixed(p),
},
})

_, err := ctx.Plan()
if err == nil {
t.Fatalf("should error")
}
}

func TestContext2Plan_moduleVarComputed(t *testing.T) {
m := testModule(t, "plan-module-var-computed")
p := testProvider("aws")
Expand Down Expand Up @@ -2035,6 +2069,7 @@ func TestContext2Plan_varListErr(t *testing.T) {
})

_, err := ctx.Plan()

if err == nil {
t.Fatal("should error")
}
Expand Down
63 changes: 63 additions & 0 deletions terraform/eval_variable.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,75 @@ package terraform

import (
"fmt"
"strings"

"github.com/hashicorp/errwrap"
"github.com/hashicorp/terraform/config"
"github.com/hashicorp/terraform/config/module"
"github.com/mitchellh/mapstructure"
)

// EvalTypeCheckVariable is an EvalNode which ensures that the variable
// values which are assigned as inputs to a module (including the root)
// match the types which are either declared for the variables explicitly
// or inferred from the default values.
//
// In order to achieve this three things are required:
// - a map of the proposed variable values
// - the configuration tree of the module in which the variable is
// declared
// - the path to the module (so we know which part of the tree to
// compare the values against).
//
// Currently since the type system is simple, we currently do not make
// use of the values since it is only valid to pass string values. The
// structure is in place for extension of the type system, however.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

type EvalTypeCheckVariable struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind doing a prose comment block summarizing the intended use case and context of the EvalNode? I've found these to be helpful when navigating unfamiliar EvalTrees, and the name is often not enough to fully get what the node is supposed to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that seems like a worthwhile effort - I think several others can be documented based on the research that went into this PR too!

Variables map[string]string
ModulePath []string
ModuleTree *module.Tree
}

func (n *EvalTypeCheckVariable) Eval(ctx EvalContext) (interface{}, error) {
currentTree := n.ModuleTree
for _, pathComponent := range n.ModulePath[1:] {
currentTree = currentTree.Children()[pathComponent]
}
targetConfig := currentTree.Config()

prototypes := make(map[string]config.VariableType)
for _, variable := range targetConfig.Variables {
prototypes[variable.Name] = variable.Type()
}

for name, declaredType := range prototypes {
// This is only necessary when we _actually_ check. It is left as a reminder
// that at the current time we are dealing with a type system consisting only
// of strings and maps - where the only valid inter-module variable type is
// string.
// proposedValue := n.Variables[name]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't end up using proposedValue it looks like.


switch declaredType {
case config.VariableTypeString:
// This will need actual verification once we aren't dealing with
// a map[string]string but this is sufficient for now.
continue
default:
// Only display a module if we are not in the root module
modulePathDescription := fmt.Sprintf(" in module %s", strings.Join(n.ModulePath[1:], "."))
if len(n.ModulePath) == 1 {
modulePathDescription = ""
}
// This will need the actual type substituting when we have more than
// just strings and maps.
return nil, fmt.Errorf("variable %s%s should be type %s, got type string",
name, modulePathDescription, declaredType.Printable())
}
}

return nil, nil
}

// EvalSetVariables is an EvalNode implementation that sets the variables
// explicitly for interpolation later.
type EvalSetVariables struct {
Expand Down
10 changes: 9 additions & 1 deletion terraform/graph_config_node_variable.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"

"github.com/hashicorp/terraform/config"
"github.com/hashicorp/terraform/config/module"
"github.com/hashicorp/terraform/dag"
)

Expand All @@ -18,7 +19,8 @@ type GraphNodeConfigVariable struct {
Module string
Value *config.RawConfig

depPrefix string
ModuleTree *module.Tree
ModulePath []string
}

func (n *GraphNodeConfigVariable) Name() string {
Expand Down Expand Up @@ -125,6 +127,12 @@ func (n *GraphNodeConfigVariable) EvalTree() EvalNode {
Variables: variables,
},

&EvalTypeCheckVariable{
Variables: variables,
ModulePath: n.ModulePath,
ModuleTree: n.ModuleTree,
},

&EvalSetVariables{
Module: &n.Module,
Variables: variables,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
variable "inner_in" {
type = "map"
default = {
us-west-1 = "ami-12345"
us-west-2 = "ami-67890"
}
}

resource "null_resource" "inner_noop" {}

output "inner_out" {
value = "${lookup(var.inner_in, "us-west-1")}"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module "middle" {
source = "./middle"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
variable "middle_in" {
type = "map"
default = {
eu-west-1 = "ami-12345"
eu-west-2 = "ami-67890"
}
}

module "inner" {
source = "../inner"

inner_in = "hello"
}

resource "null_resource" "middle_noop" {}

output "middle_out" {
value = "${lookup(var.middle_in, "us-west-1")}"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
variable "map_in" {
type = "map"
default = {
us-west-1 = "ami-12345"
us-west-2 = "ami-67890"
}
}
10 changes: 10 additions & 0 deletions terraform/test-fixtures/plan-module-wrong-var-type/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
variable "input" {
type = "string"
default = "hello world"
}

module "test" {
source = "./inner"

map_in = "${var.input}"
}
6 changes: 5 additions & 1 deletion terraform/transform_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ func (t *ConfigTransformer) Transform(g *Graph) error {

// Write all the variables out
for _, v := range config.Variables {
nodes = append(nodes, &GraphNodeConfigVariable{Variable: v})
nodes = append(nodes, &GraphNodeConfigVariable{
Variable: v,
ModuleTree: t.Module,
ModulePath: g.Path,
})
}

// Write all the provider configs out
Expand Down