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

terraform: missing provider should add missing aliases [GH-2023] #2475

Merged
merged 1 commit into from
Jun 25, 2015
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
4 changes: 4 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -683,6 +683,10 @@ func (o *Output) mergerMerge(m merger) merger {
return &result
}

func (c *ProviderConfig) GoString() string {
return fmt.Sprintf("*%#v", *c)
}

func (c *ProviderConfig) FullName() string {
if c.Alias == "" {
return c.Name
Expand Down
65 changes: 28 additions & 37 deletions terraform/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -697,43 +697,6 @@ func TestContext2Plan_preventDestroy_destroyPlan(t *testing.T) {
}
}

func TestContext2Plan_providerAliasMissing(t *testing.T) {
m := testModule(t, "apply-provider-alias-missing")
p := testProvider("aws")
p.ApplyFn = testApplyFn
p.DiffFn = testDiffFn
state := &State{
Modules: []*ModuleState{
&ModuleState{
Path: rootModulePath,
Resources: map[string]*ResourceState{
"aws_instance.foo": &ResourceState{
Type: "aws_instance",
Provider: "aws.foo",
Primary: &InstanceState{
ID: "bar",
Attributes: map[string]string{
"require_new": "abc",
},
},
},
},
},
},
}
ctx := testContext2(t, &ContextOpts{
Module: m,
Providers: map[string]ResourceProviderFactory{
"aws": testProviderFuncFixed(p),
},
State: state,
})

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

func TestContext2Plan_computed(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole test was testing the opposite of the behavior I added. It doesn't make a lot of sense to me that this should be an error, because it breaks modules completely. I think we just default it like every other provider type.

m := testModule(t, "plan-computed")
p := testProvider("aws")
Expand Down Expand Up @@ -4375,6 +4338,34 @@ func TestContext2Apply_moduleDestroyOrder(t *testing.T) {
}
}

func TestContext2Apply_moduleProviderAlias(t *testing.T) {
m := testModule(t, "apply-module-provider-alias")
p := testProvider("aws")
p.ApplyFn = testApplyFn
p.DiffFn = testDiffFn
ctx := testContext2(t, &ContextOpts{
Module: m,
Providers: map[string]ResourceProviderFactory{
"aws": testProviderFuncFixed(p),
},
})

if _, err := ctx.Plan(); err != nil {
t.Fatalf("err: %s", err)
}

state, err := ctx.Apply()
if err != nil {
t.Fatalf("err: %s", err)
}

actual := strings.TrimSpace(state.String())
expected := strings.TrimSpace(testTerraformApplyModuleProviderAliasStr)
if actual != expected {
t.Fatalf("bad: \n%s", actual)
}
}

func TestContext2Apply_moduleVarResourceCount(t *testing.T) {
m := testModule(t, "apply-module-var-resource-count")
p := testProvider("aws")
Expand Down
8 changes: 8 additions & 0 deletions terraform/terraform_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,14 @@ do_instance.foo:
type = do_instance
`

const testTerraformApplyModuleProviderAliasStr = `
<no state>
module.child:
aws_instance.foo:
ID = foo
provider = aws.eu
`

const testTerraformApplyOutputOrphanStr = `
<no state>
Outputs:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
provider "aws" {
alias = "eu"
}

resource "aws_instance" "foo" {
provider = "aws.eu"
}
3 changes: 3 additions & 0 deletions terraform/test-fixtures/apply-module-provider-alias/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module "child" {
source = "./child"
}
8 changes: 0 additions & 8 deletions terraform/test-fixtures/apply-provider-alias-missing/main.tf

This file was deleted.

3 changes: 3 additions & 0 deletions terraform/test-fixtures/transform-provider-missing/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
provider "aws" {}
resource "aws_instance" "web" {}
resource "foo_instance" "web" {}
40 changes: 35 additions & 5 deletions terraform/transform_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package terraform

import (
"fmt"
"strings"

"github.com/hashicorp/go-multierror"
"github.com/hashicorp/terraform/config"
Expand Down Expand Up @@ -146,15 +147,44 @@ type MissingProviderTransformer struct {
}

func (t *MissingProviderTransformer) Transform(g *Graph) error {
// Create a set of our supported providers
supported := make(map[string]struct{}, len(t.Providers))
for _, v := range t.Providers {
supported[v] = struct{}{}
}

// Get the map of providers we already have in our graph
m := providerVertexMap(g)
for _, p := range t.Providers {
if _, ok := m[p]; ok {
// This provider already exists as a configured node

// Go through all the provider consumers and make sure we add
// that provider if it is missing.
for _, v := range g.Vertices() {
pv, ok := v.(GraphNodeProviderConsumer)
if !ok {
continue
}

// Add our own missing provider node to the graph
g.Add(&graphNodeMissingProvider{ProviderNameValue: p})
for _, p := range pv.ProvidedBy() {
if _, ok := m[p]; ok {
// This provider already exists as a configure node
break
}

// If the provider has an alias in it, we just want the type
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we know the provider has an alias in it here?

At first I was worried about what would happen if somebody did provider = "aws.oh.whoops", but I think it's fine - the graphNodeMissingProvider will get added with a weird ProviderNameValue and fail later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, we just check for the "." and assume it exists because proper smeantic checks like that should happen earlier.

ptype := p
if idx := strings.IndexRune(p, '.'); idx != -1 {
ptype = p[:idx]
}

if _, ok := supported[ptype]; !ok {
// If we don't support the provider type, skip it.
// Validation later will catch this as an error.
continue
}

// Add our own missing provider node to the graph
m[p] = g.Add(&graphNodeMissingProvider{ProviderNameValue: p})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the key change. Before we would just blindly add every supported provider as a missing provider. But now we actually go through, find what providers we need, and add them each in turn.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 I like this more direct approach.

}

return nil
Expand Down
7 changes: 5 additions & 2 deletions terraform/transform_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func TestCloseProviderTransformer(t *testing.T) {
}

func TestMissingProviderTransformer(t *testing.T) {
mod := testModule(t, "transform-provider-basic")
mod := testModule(t, "transform-provider-missing")

g := Graph{Path: RootModulePath}
{
Expand All @@ -74,7 +74,7 @@ func TestMissingProviderTransformer(t *testing.T) {
}

{
transform := &MissingProviderTransformer{Providers: []string{"foo"}}
transform := &MissingProviderTransformer{Providers: []string{"foo", "bar"}}
if err := transform.Transform(&g); err != nil {
t.Fatalf("err: %s", err)
}
Expand Down Expand Up @@ -275,10 +275,13 @@ provider.aws (close)

const testTransformMissingProviderBasicStr = `
aws_instance.web
foo_instance.web
provider.aws
provider.aws (close)
aws_instance.web
provider.foo
provider.foo (close)
foo_instance.web
`

const testTransformPruneProviderBasicStr = `
Expand Down