Merge pull request #379 from hashicorp/f-deprecate-weird-names

Warn/error names with special characters

config/config.go

@@ -4,6 +4,7 @@ package config
 
 import (
 	"fmt"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -13,6 +14,10 @@ import (
 	"github.com/mitchellh/reflectwalk"
 )
 
+// NameRegexp is the regular expression that all names (modules, providers,
+// resources, etc.) must follow.
+var NameRegexp = regexp.MustCompile(`\A[A-Za-z0-9\-\_]+\z`)
+
 // Config is the configuration that comes from loading a collection
 // of Terraform templates.
 type Config struct {
@@ -236,9 +241,9 @@ func (c *Config) Validate() error {
 			}
 		}
 
-		// If we haven't seen this module before, check that the
-		// source has no interpolations.
 		if _, ok := modules[m.Id()]; !ok {
+			// If we haven't seen this module before, check that the
+			// source has no interpolations.
 			rc, err := NewRawConfig(map[string]interface{}{
 				"root": m.Source,
 			})
@@ -251,6 +256,14 @@ func (c *Config) Validate() error {
 					"%s: module source cannot contain interpolations",
 					m.Id()))
 			}
+
+			// Check that the name matches our regexp
+			if !NameRegexp.Match([]byte(m.Name)) {
+				errs = append(errs, fmt.Errorf(
+					"%s: module name can only contain letters, numbers, "+
+						"dashes, and underscores",
+					m.Id()))
+			}
 		}
 
 		modules[m.Id()] = m

config/config_test.go

@@ -123,6 +123,13 @@ func TestConfigValidate_dupResource(t *testing.T) {
 	}
 }
 
+func TestConfigValidate_moduleNameBad(t *testing.T) {
+	c := testConfig(t, "validate-module-name-bad")
+	if err := c.Validate(); err == nil {
+		t.Fatal("should not be valid")
+	}
+}
+
 func TestConfigValidate_moduleSourceVar(t *testing.T) {
 	c := testConfig(t, "validate-module-source-var")
 	if err := c.Validate(); err == nil {
@@ -228,6 +235,26 @@ func TestConfigValidate_varModuleInvalid(t *testing.T) {
 	}
 }
 
+func TestNameRegexp(t *testing.T) {
+	cases := []struct{
+		Input string
+		Match bool
+	}{
+		{"hello", true},
+		{"foo-bar", true},
+		{"foo_bar", true},
+		{"_hello", true},
+		{"foo bar", false},
+		{"foo.bar", false},
+	}
+
+	for _, tc := range cases {
+		if NameRegexp.Match([]byte(tc.Input)) != tc.Match {
+			t.Fatalf("Input: %s\n\nExpected: %#v", tc.Input, tc.Match)
+		}
+	}
+}
+
 func TestProviderConfigName(t *testing.T) {
 	pcs := []*ProviderConfig{
 		&ProviderConfig{Name: "aw"},

config/test-fixtures/validate-module-name-bad/main.tf

@@ -0,0 +1,3 @@
+module "foo bar" {
+    source = "foo"
+}

terraform/context.go

@@ -1049,6 +1049,18 @@ func (c *walkContext) validateWalkFn() depgraph.WalkFunc {
 				return nil
 			}
 
+			// If the resouce name doesn't match the name regular
+			// expression, show a warning.
+			if !config.NameRegexp.Match([]byte(rn.Config.Name)) {
+				l.Lock()
+				meta.Warns = append(meta.Warns, fmt.Sprintf(
+					"%s: module name can only contain letters, numbers, "+
+						"dashes, and underscores.\n"+
+						"This will be an error in Terraform 0.4",
+					rn.Resource.Id))
+				l.Unlock()
+			}
+
 			log.Printf("[INFO] Validating resource: %s", rn.Resource.Id)
 			ws, es := rn.Resource.Provider.ValidateResource(
 				rn.Resource.Info.Type, rn.Resource.Config)

terraform/context_test.go

@@ -312,6 +312,25 @@ func TestContextValidate_resourceConfig_good(t *testing.T) {
 	}
 }
 
+func TestContextValidate_resourceNameSymbol(t *testing.T) {
+	p := testProvider("aws")
+	m := testModule(t, "validate-resource-name-symbol")
+	c := testContext(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+	})
+
+	w, e := c.Validate()
+	if len(w) == 0 {
+		t.Fatalf("bad: %#v", w)
+	}
+	if len(e) > 0 {
+		t.Fatalf("bad: %#v", e)
+	}
+}
+
 func TestContextValidate_requiredVar(t *testing.T) {
 	m := testModule(t, "validate-required-var")
 	c := testContext(t, &ContextOpts{

terraform/test-fixtures/validate-resource-name-symbol/main.tf

@@ -0,0 +1,3 @@
+resource "aws_instance" "foo bar" {
+    num = "2"
+}