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

Allow targeting orphan nodes #3912

Merged
merged 2 commits into from
Nov 13, 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
47 changes: 47 additions & 0 deletions terraform/context_plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1627,6 +1627,53 @@ STATE:
}
}

func TestContext2Plan_targetedOrphan(t *testing.T) {
m := testModule(t, "plan-targeted-orphan")
p := testProvider("aws")
p.DiffFn = testDiffFn
ctx := testContext2(t, &ContextOpts{
Module: m,
Providers: map[string]ResourceProviderFactory{
"aws": testProviderFuncFixed(p),
},
State: &State{
Modules: []*ModuleState{
&ModuleState{
Path: rootModulePath,
Resources: map[string]*ResourceState{
"aws_instance.orphan": &ResourceState{
Type: "aws_instance",
Primary: &InstanceState{
ID: "i-789xyz",
},
},
},
},
},
},
Destroy: true,
Targets: []string{"aws_instance.orphan"},
})

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

actual := strings.TrimSpace(plan.String())
expected := strings.TrimSpace(`DIFF:

DESTROY: aws_instance.orphan

STATE:

aws_instance.orphan:
ID = i-789xyz`)
if actual != expected {
t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual)
}
}

func TestContext2Plan_provider(t *testing.T) {
m := testModule(t, "plan-provider")
p := testProvider("aws")
Expand Down
5 changes: 2 additions & 3 deletions terraform/graph_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,8 @@ func (b *BuiltinGraphBuilder) Steps(path []string) []GraphTransformer {
// Create all our resources from the configuration and state
&ConfigTransformer{Module: b.Root},
&OrphanTransformer{
State: b.State,
Module: b.Root,
Targeting: len(b.Targets) > 0,
State: b.State,
Module: b.Root,
},

// Output-related transformations
Expand Down
6 changes: 3 additions & 3 deletions terraform/graph_config_node_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,9 @@ func (n *GraphNodeConfigResource) DynamicExpand(ctx EvalContext) (*Graph, error)
// expand orphans, which have all the same semantics in a destroy
// as a primary.
steps = append(steps, &OrphanTransformer{
State: state,
View: n.Resource.Id(),
Targeting: len(n.Targets) > 0,
State: state,
View: n.Resource.Id(),
Targets: n.Targets,
})

steps = append(steps, &DeposedTransformer{
Expand Down
6 changes: 6 additions & 0 deletions terraform/test-fixtures/plan-targeted-orphan/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# This resource was previously "created" and the fixture represents
# it being destroyed subsequently

/*resource "aws_instance" "orphan" {*/
/*foo = "bar"*/
/*}*/
33 changes: 23 additions & 10 deletions terraform/transform_orphan.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package terraform

import (
"fmt"
"log"
"strings"

"github.com/hashicorp/terraform/config"
"github.com/hashicorp/terraform/config/module"
Expand All @@ -29,7 +29,7 @@ type OrphanTransformer struct {
// Targets are user-specified resources to target. We need to be aware of
// these so we don't improperly identify orphans when they've just been
// filtered out of the graph via targeting.
Targeting bool
Targets []ResourceAddress

// View, if non-nil will set a view on the module state.
View string
Expand All @@ -41,13 +41,6 @@ func (t *OrphanTransformer) Transform(g *Graph) error {
return nil
}

if t.Targeting {
log.Printf("Skipping orphan transformer because we have targets.")
// If we are in a run where we are targeting nodes, we won't process
// orphans for this run.
return nil
}

// Build up all our state representatives
resourceRep := make(map[string]struct{})
for _, v := range g.Vertices() {
Expand All @@ -74,8 +67,24 @@ func (t *OrphanTransformer) Transform(g *Graph) error {
state = state.View(t.View)
}

// Go over each resource orphan and add it to the graph.
resourceOrphans := state.Orphans(config)
if len(t.Targets) > 0 {
var targetedOrphans []string
for _, o := range resourceOrphans {
targeted := false
for _, t := range t.Targets {
prefix := fmt.Sprintf("%s.%s.%d", t.Type, t.Name, t.Index)
if strings.HasPrefix(o, prefix) {
targeted = true
}
}
if targeted {
targetedOrphans = append(targetedOrphans, o)
}
}
resourceOrphans = targetedOrphans
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think going about this filter in the other direction might be a bit more straightforward. Let's see how it looks:

resourceOrphans := state.Orphans(config)
if len(t.Targets) > 0 {
  var targetedOrphans []string
  for _, o := range resourceOrphans {
    targeted := false
    for _, t := range t.Targets {
      prefix := fmt.Sprintf("%s.%s.%d", t.Type, t.Name, t.Index)
      if strings.HasPrefix(o, prefix) {
        targeted = true
      }
    }
    if targeted {
      targetedOrphans = append(targetedOrphans, o)
    }
  }
  resourceOrphans = targetedOrphans
}

Seems sort of nice to have the targeting behavior all behind the guard and the normal case is the straight-through code path. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likely better, I'll update.


resourceVertexes = make([]dag.Vertex, len(resourceOrphans))
for i, k := range resourceOrphans {
// If this orphan is represented by some other node somehow,
Expand Down Expand Up @@ -173,6 +182,10 @@ type graphNodeOrphanResource struct {
dependentOn []string
}

func (n *graphNodeOrphanResource) ResourceAddress() *ResourceAddress {
return n.ResourceAddress()
}

func (n *graphNodeOrphanResource) DependableName() []string {
return []string{n.dependableName()}
}
Expand Down