Repository: chanzuckerberg/fogg
Pull request #297 (2019-06-14 18:40:51)

refactor: move tools to common
Before this tools could only be defined at the top level (not per-component).

We would like to be able to configure Atlantis separately for each component. As a bonus this will allow us to do the same for the other tools.

Note that this is a breaking change for config v2, but given that it isn't deployed widely I think we can get away with it.

### Test Plan
* unit tests

### References
*


codecov[bot]: # [Codecov](https://codecov.io/gh/chanzuckerberg/fogg/pull/297?src=pr&el=h1) Report
> Merging [#297](https://codecov.io/gh/chanzuckerberg/fogg/pull/297?src=pr&el=desc) into [master](https://codecov.io/gh/chanzuckerberg/fogg/commit/82748617aab2997e63f2c465e6e0de2c6fc7101c?src=pr&el=desc) will **increase** coverage by `0.18%`.
> The diff coverage is `96.42%`.

[![Impacted file tree graph](https://codecov.io/gh/chanzuckerberg/fogg/pull/297/graphs/tree.svg?width=650&token=57DhtZ7g2A&height=150&src=pr)](https://codecov.io/gh/chanzuckerberg/fogg/pull/297?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   79.91%   80.09%   +0.18%     
==========================================
  Files          17       17              
  Lines        1399     1422      +23     
==========================================
+ Hits         1118     1139      +21     
- Misses        160      162       +2     
  Partials      121      121
```


| [Impacted Files](https://codecov.io/gh/chanzuckerberg/fogg/pull/297?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [config/config.go](https://codecov.io/gh/chanzuckerberg/fogg/pull/297/diff?src=pr&el=tree#diff-Y29uZmlnL2NvbmZpZy5nbw==) | `94.04% <100%> (-0.04%)` | :arrow_down: |
| [plan/plan.go](https://codecov.io/gh/chanzuckerberg/fogg/pull/297/diff?src=pr&el=tree#diff-cGxhbi9wbGFuLmdv) | `90.96% <100%> (-0.75%)` | :arrow_down: |
| [config/v2/config.go](https://codecov.io/gh/chanzuckerberg/fogg/pull/297/diff?src=pr&el=tree#diff-Y29uZmlnL3YyL2NvbmZpZy5nbw==) | `100% <100%> (ø)` | :arrow_up: |
| [config/v2/resolvers.go](https://codecov.io/gh/chanzuckerberg/fogg/pull/297/diff?src=pr&el=tree#diff-Y29uZmlnL3YyL3Jlc29sdmVycy5nbw==) | `98.77% <100%> (+0.05%)` | :arrow_up: |
| [plan/travisci.go](https://codecov.io/gh/chanzuckerberg/fogg/pull/297/diff?src=pr&el=tree#diff-cGxhbi90cmF2aXNjaS5nbw==) | `94.44% <100%> (+0.32%)` | :arrow_up: |
| [config/v2/validation.go](https://codecov.io/gh/chanzuckerberg/fogg/pull/297/diff?src=pr&el=tree#diff-Y29uZmlnL3YyL3ZhbGlkYXRpb24uZ28=) | `81.81% <91.66%> (+1.81%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/chanzuckerberg/fogg/pull/297?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/chanzuckerberg/fogg/pull/297?src=pr&el=footer). Last update [8274861...8dd5edf](https://codecov.io/gh/chanzuckerberg/fogg/pull/297?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/config/config.go b/config/config.go
index 4ec04917..c35cd134 100644
--- a/config/config.go
+++ b/config/config.go
@@ -130,18 +130,13 @@ func UpgradeConfigVersion(c1 *v1.Config) (*v2.Config, error) {
 			Owner:            util.StrPtr(def1.Owner),
 			Project:          util.StrPtr(def1.Project),
 			TerraformVersion: util.StrPtr(def1.TerraformVersion),
+			Tools: &v2.Tools{
+				TfLint:   def1.TfLint,
+				TravisCI: c1.TravisCI,
+			},
 		},
 	}
 
-	c2.Tools = v2.Tools{}
-	if def1.TfLint != nil {
-		c2.Tools.TfLint = def1.TfLint
-	}
-
-	if c1.TravisCI != nil {
-		c2.Tools.TravisCI = c1.TravisCI
-	}
-
 	for acctName, acct := range c1.Accounts {
 		common := v2.Common{
 			ExtraVars: acct.ExtraVars,
diff --git a/config/v2/config.go b/config/v2/config.go
index a8074a5e..8bdb7ed2 100644
--- a/config/v2/config.go
+++ b/config/v2/config.go
@@ -27,7 +27,6 @@ type Config struct {
 	Global   Component            `json:"global,omitempty"`
 	Modules  map[string]v1.Module `json:"modules,omitempty"`
 	Plugins  v1.Plugins           `json:"plugins,omitempty"`
-	Tools    Tools                `json:"tools,omitempty"`
 	Version  int                  `json:"version" validate:"required,eq=2"`
 }
 
@@ -38,6 +37,7 @@ type Common struct {
 	Project          *string           `json:"project,omitempty" `
 	Providers        *Providers        `json:"providers,omitempty" `
 	TerraformVersion *string           `json:"terraform_version,omitempty"`
+	Tools            *Tools            `json:"tools,omitempty"`
 }
 
 type Defaults struct {
@@ -203,6 +203,23 @@ func (c *Config) Generate(r *rand.Rand, size int) reflect.Value {
 			},
 			TerraformVersion: randStringPtr(r, s),
 		}
+
+		if r.Float32() < 0.5 {
+			c.Tools = &Tools{}
+			if r.Float32() < 0.5 {
+				c.Tools.TravisCI = &v1.TravisCI{
+					Enabled:     r.Float32() < 0.5,
+					TestBuckets: r.Intn(size),
+				}
+			}
+			if r.Float32() < 0.5 {
+				p := r.Float32() < 0.5
+				c.Tools.TfLint = &v1.TfLint{
+					Enabled: &p,
+				}
+			}
+		}
+
 		return c
 	}
 
@@ -214,15 +231,6 @@ func (c *Config) Generate(r *rand.Rand, size int) reflect.Value {
 
 	// tools
 
-	conf.Tools = Tools{}
-
-	if r.Float32() < 0.5 {
-		conf.Tools.TravisCI = &v1.TravisCI{
-			Enabled:     r.Float32() < 0.5,
-			TestBuckets: r.Intn(size),
-		}
-	}
-
 	conf.Accounts = map[string]Account{}
 	acctN := r.Intn(size)
 
diff --git a/config/v2/resolvers.go b/config/v2/resolvers.go
index 1f21bbbe..93d27f57 100644
--- a/config/v2/resolvers.go
+++ b/config/v2/resolvers.go
@@ -135,6 +135,19 @@ func ResolveBlessProvider(commons ...Common) *BlessProvider {
 	}
 }
 
+func ResolveTfLint(commons ...Common) v1.TfLint {
+	enabled := false
+	for _, c := range commons {
+		if c.Tools != nil && c.Tools.TfLint != nil && c.Tools.TfLint.Enabled != nil {
+			enabled = *c.Tools.TfLint.Enabled
+		}
+	}
+
+	return v1.TfLint{
+		Enabled: &enabled,
+	}
+}
+
 func OwnerGetter(comm Common) *string {
 	return comm.Owner
 }
diff --git a/config/v2/validation.go b/config/v2/validation.go
index bba6893a..cc2f00b4 100644
--- a/config/v2/validation.go
+++ b/config/v2/validation.go
@@ -48,6 +48,14 @@ func (c *Config) Validate() ([]string, error) {
 	errs = multierror.Append(errs, c.ValidateBlessProviders())
 	errs = multierror.Append(errs, c.validateModules())
 
+	// refactor to make it easier to manage these
+	w, e := c.ValidateToolsTravis()
+	warnings = append(warnings, w...)
+	errs = multierror.Append(errs, e)
+	w, e = c.ValidateToolsTfLint()
+	warnings = append(warnings, w...)
+	errs = multierror.Append(errs, e)
+
 	if c.Docker {
 		warnings = append(warnings, "Docker support is no longer supported and the config entry is ignored.")
 	}
@@ -55,6 +63,10 @@ func (c *Config) Validate() ([]string, error) {
 	return warnings, errs.ErrorOrNil()
 }
 
+func merge(warnings []string, err *multierror.Error, w []string, e error) ([]string, *multierror.Error) {
+	return append(warnings, w...), multierror.Append(err, e)
+}
+
 func ValidateAWSProvider(p *AWSProvider, component string) error {
 	var errs *multierror.Error
 	if p == nil {
@@ -152,6 +164,31 @@ func (c *Config) ValidateBlessProviders() error {
 	return errs
 }
 
+func (c *Config) ValidateToolsTravis() ([]string, error) {
+	var warns []string
+	var errs *multierror.Error
+	c.WalkComponents(func(component string, comms ...Common) {
+		c := comms[len(comms)-1]
+		if c.Tools != nil && c.Tools.TravisCI != nil {
+			warns = append(warns, fmt.Sprintf("per-component travisci config is not implemented, ignoring config in %s", component))
+		}
+	})
+
+	return warns, errs
+}
+
+func (c *Config) ValidateToolsTfLint() ([]string, error) {
+	var warns []string
+	var errs *multierror.Error
+	c.WalkComponents(func(component string, comms ...Common) {
+		c := comms[len(comms)-1]
+		if c.Tools != nil && c.Tools.TfLint != nil {
+			warns = append(warns, fmt.Sprintf("per-component tflint config is not implemented, ignoring config in %s", component))
+		}
+	})
+
+	return warns, errs
+}
 func (c *Config) WalkComponents(f func(component string, commons ...Common)) {
 	for name, acct := range c.Accounts {
 		f(fmt.Sprintf("accounts/%s", name), c.Defaults.Common, acct.Common)
diff --git a/plan/plan.go b/plan/plan.go
index 63164aaa..1994bd13 100644
--- a/plan/plan.go
+++ b/plan/plan.go
@@ -137,9 +137,7 @@ func Eval(c *v2.Config) (*Plan, error) {
 	}
 	p.Modules = p.buildModules(c)
 
-	if c.Tools.TravisCI != nil {
-		p.TravisCI = p.buildTravisCI(c, v)
-	}
+	p.TravisCI = p.buildTravisCI(c, v)
 
 	return p, nil
 }
@@ -163,7 +161,6 @@ func (p *Plan) buildAccounts(c *v2.Config) map[string]Account {
 
 		accountPlan.AccountName = name
 		accountPlan.ComponentCommon = resolveComponentCommon(defaults.Common, acct.Common)
-		accountPlan.TfLint = resolveTfLint(c.Tools.TfLint, nil)
 		accountPlan.AllAccounts = resolveAccounts(c.Accounts)
 		accountPlan.PathToRepoRoot = "../../../"
 		accountPlan.Global = &p.Global
@@ -206,8 +203,6 @@ func (p *Plan) buildGlobal(conf *v2.Config) Component {
 	componentPlan.ExtraVars = resolveExtraVars(defaults.ExtraVars, global.ExtraVars)
 	componentPlan.PathToRepoRoot = "../../"
 
-	componentPlan.TfLint = resolveTfLint(conf.Tools.TfLint, nil)
-
 	return componentPlan
 }
 
@@ -242,7 +237,6 @@ func (p *Plan) buildEnvs(conf *v2.Config) (map[string]Env, error) {
 			componentPlan.ModuleSource = componentConf.ModuleSource
 			componentPlan.PathToRepoRoot = "../../../../"
 
-			componentPlan.TfLint = resolveTfLint(conf.Tools.TfLint, nil)
 			componentPlan.Global = &p.Global
 
 			envPlan.Components[componentName] = componentPlan
@@ -289,6 +283,12 @@ func resolveComponentCommon(commons ...v2.Common) ComponentCommon {
 		}
 	}
 
+	tflintConfig := v2.ResolveTfLint(commons...)
+
+	tfLintPlan := TfLint{
+		Enabled: *tflintConfig.Enabled,
+	}
+
 	return ComponentCommon{
 		Backend: AWSBackend{
 			Region:      v2.ResolveRequiredString(v2.BackendRegionGetter, commons...),
@@ -301,6 +301,7 @@ func resolveComponentCommon(commons ...v2.Common) ComponentCommon {
 			Snowflake: snowflakePlan,
 			Bless:     blessPlan,
 		},
+		TfLint:    tfLintPlan,
 		ExtraVars: v2.ResolveStringMap(v2.ExtraVarsGetter, commons...),
 		Owner:     v2.ResolveRequiredString(v2.OwnerGetter, commons...),
 		Project:   v2.ResolveRequiredString(v2.ProjectGetter, commons...),
@@ -353,27 +354,3 @@ func resolveAccounts(accounts map[string]v2.Account) map[string]int64 {
 	}
 	return a
 }
-
-func resolveTfLint(def *v1.TfLint, override *v1.TfLint) TfLint {
-	enabled := false
-	if def != nil && def.Enabled != nil {
-		enabled = *def.Enabled
-	}
-	if override != nil && override.Enabled != nil {
-		enabled = *override.Enabled
-	}
-	return TfLint{
-		Enabled: enabled,
-	}
-}
-
-func resolveTfLintComponent(def TfLint, override *v1.TfLint) TfLint {
-
-	enabled := def.Enabled
-	if override != nil && override.Enabled != nil {
-		enabled = *override.Enabled
-	}
-	return TfLint{
-		Enabled: enabled,
-	}
-}
diff --git a/plan/travisci.go b/plan/travisci.go
index f187d29e..59638274 100644
--- a/plan/travisci.go
+++ b/plan/travisci.go
@@ -25,8 +25,12 @@ func (p *Plan) buildTravisCI(c *v2.Config, version string) TravisCI {
 		panic("buildTravisCI must be run after buildAccounts")
 	}
 
+	if c.Defaults.Tools == nil || c.Defaults.Tools.TravisCI == nil {
+		return TravisCI{}
+	}
+
 	tr := TravisCI{
-		Enabled: c.Tools.TravisCI.Enabled,
+		Enabled: c.Defaults.Tools.TravisCI.Enabled,
 	}
 	var profiles []AWSProfile
 
@@ -38,14 +42,14 @@ func (p *Plan) buildTravisCI(c *v2.Config, version string) TravisCI {
 			// TODO since accountID is required here, that means we need
 			// to make it non-optional, either in defaults or post-plan.
 			ID:   p.Accounts[name].Providers.AWS.AccountID,
-			Role: c.Tools.TravisCI.AWSIAMRoleName,
+			Role: c.Defaults.Tools.TravisCI.AWSIAMRoleName,
 		})
 	}
 	tr.AWSProfiles = profiles
 
 	var buckets int
-	if c.Tools.TravisCI.TestBuckets > 0 {
-		buckets = c.Tools.TravisCI.TestBuckets
+	if c.Defaults.Tools.TravisCI.TestBuckets > 0 {
+		buckets = c.Defaults.Tools.TravisCI.TestBuckets
 	} else {
 		buckets = 1
 	}


Test patch:
diff --git a/config/config_test.go b/config/config_test.go
index 29194e7f..b58072da 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -99,18 +99,19 @@ func TestUpgradeConfigVersion(t *testing.T) {
 				Project:          util.StrPtr("test-project"),
 				ExtraVars:        map[string]string{"foo": "bar"},
 				TerraformVersion: util.StrPtr("0.11.0"),
+				Tools: &v2.Tools{
+					TfLint: &v1.TfLint{
+						Enabled: boolptr(true),
+					},
+					TravisCI: &v1.TravisCI{
+						Enabled:        true,
+						AWSIAMRoleName: "travis-role",
+						TestBuckets:    13,
+					},
+				},
 			},
 		},
-		Tools: v2.Tools{
-			TfLint: &v1.TfLint{
-				Enabled: boolptr(true),
-			},
-			TravisCI: &v1.TravisCI{
-				Enabled:        true,
-				AWSIAMRoleName: "travis-role",
-				TestBuckets:    13,
-			},
-		},
+
 		Accounts: map[string]v2.Account{
 			"foo": v2.Account{
 				Common: v2.Common{
diff --git a/config/v2/resolvers_test.go b/config/v2/resolvers_test.go
new file mode 100644
index 00000000..fd15aec8
--- /dev/null
+++ b/config/v2/resolvers_test.go
@@ -0,0 +1,39 @@
+package v2_test
+
+import (
+	"testing"
+
+	"github.com/chanzuckerberg/fogg/config/v1"
+	"github.com/chanzuckerberg/fogg/config/v2"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestResolveTfLint(test *testing.T) {
+	a := assert.New(test)
+	t := true
+	f := false
+
+	data := []struct {
+		def    *bool
+		over   *bool
+		output *bool
+	}{
+		{nil, nil, &f},
+		{nil, &t, &t},
+		{nil, &f, &f},
+		{&t, nil, &t},
+		{&t, &t, &t},
+		{&t, &f, &f},
+		{&f, nil, &f},
+		{&f, &t, &t},
+		{&f, &f, &f},
+	}
+	for _, r := range data {
+		test.Run("", func(t *testing.T) {
+			def := v2.Common{Tools: &v2.Tools{TfLint: &v1.TfLint{Enabled: r.def}}}
+			over := v2.Common{Tools: &v2.Tools{TfLint: &v1.TfLint{Enabled: r.over}}}
+			result := v2.ResolveTfLint(def, over)
+			a.Equal(r.output, result.Enabled)
+		})
+	}
+}
diff --git a/plan/plan_test.go b/plan/plan_test.go
index f07aade2..bb541649 100644
--- a/plan/plan_test.go
+++ b/plan/plan_test.go
@@ -166,63 +166,6 @@ func TestExtraVarsCompositionV2(t *testing.T) {
 
 }
 
-func TestResolveTfLint(test *testing.T) {
-	a := assert.New(test)
-	t := true
-	f := false
-
-	data := []struct {
-		def    *bool
-		over   *bool
-		output bool
-	}{
-		{nil, nil, false},
-		{nil, &t, true},
-		{nil, &f, false},
-		{&t, nil, true},
-		{&t, &t, true},
-		{&t, &f, false},
-		{&f, nil, false},
-		{&f, &t, true},
-		{&f, &f, false},
-	}
-	for _, r := range data {
-		test.Run("", func(t *testing.T) {
-			def := &v1.TfLint{Enabled: r.def}
-			over := &v1.TfLint{Enabled: r.over}
-			result := resolveTfLint(def, over)
-			a.Equal(r.output, result.Enabled)
-		})
-	}
-}
-
-func TestResolveTfLintComponent(test *testing.T) {
-	a := assert.New(test)
-	t := true
-	f := false
-
-	data := []struct {
-		def    bool
-		over   *bool
-		output bool
-	}{
-		{t, nil, true},
-		{t, &t, true},
-		{t, &f, false},
-		{f, nil, false},
-		{f, &t, true},
-		{f, &f, false},
-	}
-	for _, r := range data {
-		test.Run("", func(t *testing.T) {
-			def := TfLint{Enabled: r.def}
-			over := &v1.TfLint{Enabled: r.over}
-			result := resolveTfLintComponent(def, over)
-			a.Equal(r.output, result.Enabled)
-		})
-	}
-}
-
 func TestResolveEKSConfig(t *testing.T) {
 	a := assert.New(t)
 	a.Equal("", resolveEKSConfig(nil, nil).ClusterName)
diff --git a/plan/travisci_test.go b/plan/travisci_test.go
index 147f6863..cc08f1a3 100644
--- a/plan/travisci_test.go
+++ b/plan/travisci_test.go
@@ -20,9 +20,13 @@ func Test_buildTravisCI_Disabled(t *testing.T) {
 	a := assert.New(t)
 	{
 		c := &v2.Config{
-			Tools: v2.Tools{
-				TravisCI: &v1.TravisCI{
-					Enabled: false,
+			Defaults: v2.Defaults{
+				Common: v2.Common{
+					Tools: &v2.Tools{
+						TravisCI: &v1.TravisCI{
+							Enabled: false,
+						},
+					},
 				},
 			},
 		}
@@ -57,6 +61,10 @@ func Test_buildTravisCI_Profiles(t *testing.T) {
 					Region:  util.StrPtr("us-west-2"),
 					Profile: util.StrPtr("profile"),
 				},
+				Tools: &v2.Tools{TravisCI: &v1.TravisCI{
+					Enabled:        true,
+					AWSIAMRoleName: "rollin",
+				}},
 			},
 		},
 		Accounts: map[string]v2.Account{
@@ -64,10 +72,6 @@ func Test_buildTravisCI_Profiles(t *testing.T) {
 				Common: v2.Common{Providers: &v2.Providers{AWS: &v2.AWSProvider{AccountID: &id1}}},
 			},
 		},
-		Tools: v2.Tools{TravisCI: &v1.TravisCI{
-			Enabled:        true,
-			AWSIAMRoleName: "rollin",
-		}},
 	}
 
 	w, err := c.Validate()
@@ -106,6 +110,10 @@ func Test_buildTravisCI_TestBuckets(t *testing.T) {
 					Region:  util.StrPtr("us-west-2"),
 					Profile: util.StrPtr("profile"),
 				},
+				Tools: &v2.Tools{TravisCI: &v1.TravisCI{
+					Enabled:        true,
+					AWSIAMRoleName: "rollin",
+				}},
 			},
 		},
 		Accounts: map[string]v2.Account{
@@ -116,10 +124,6 @@ func Test_buildTravisCI_TestBuckets(t *testing.T) {
 				Common: v2.Common{Providers: &v2.Providers{AWS: &v2.AWSProvider{AccountID: &id2}}},
 			},
 		},
-		Tools: v2.Tools{TravisCI: &v1.TravisCI{
-			Enabled:        true,
-			AWSIAMRoleName: "rollin",
-		}},
 	}
 
 	w, err := c.Validate()