-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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: destroy node should not create #1046
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,6 +238,38 @@ func (n *EvalDiffTainted) Eval(ctx EvalContext) (interface{}, error) { | |
return nil, nil | ||
} | ||
|
||
// EvalFilterDiff is an EvalNode implementation that filters the diff | ||
// according to some filter. | ||
type EvalFilterDiff struct { | ||
// Input and output | ||
Diff **InstanceDiff | ||
Output **InstanceDiff | ||
|
||
// Destroy, if true, will only include a destroy diff if it is set. | ||
Destroy bool | ||
} | ||
|
||
func (n *EvalFilterDiff) Eval(ctx EvalContext) (interface{}, error) { | ||
if *n.Diff == nil { | ||
return nil, nil | ||
} | ||
|
||
input := *n.Diff | ||
result := new(InstanceDiff) | ||
|
||
if n.Destroy { | ||
if input.Destroy || input.RequiresNew() { | ||
result.Destroy = true | ||
} | ||
} | ||
|
||
if n.Output != nil { | ||
*n.Output = result | ||
} | ||
|
||
return nil, nil | ||
} | ||
|
||
// EvalReadDiff is an EvalNode implementation that writes the diff to | ||
// the full diff. | ||
type EvalReadDiff struct { | ||
|
@@ -275,7 +307,10 @@ func (n *EvalWriteDiff) Eval(ctx EvalContext) (interface{}, error) { | |
diff, lock := ctx.Diff() | ||
|
||
// The diff to write, if its empty it should write nil | ||
diffVal := *n.Diff | ||
var diffVal *InstanceDiff | ||
if n.Diff != nil { | ||
diffVal = *n.Diff | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change allows us to |
||
if diffVal.Empty() { | ||
diffVal = nil | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
package terraform | ||
|
||
import ( | ||
"reflect" | ||
"testing" | ||
) | ||
|
||
func TestEvalFilterDiff(t *testing.T) { | ||
ctx := new(MockEvalContext) | ||
|
||
cases := []struct { | ||
Node *EvalFilterDiff | ||
Input *InstanceDiff | ||
Output *InstanceDiff | ||
}{ | ||
// With no settings, it returns an empty diff | ||
{ | ||
&EvalFilterDiff{}, | ||
&InstanceDiff{Destroy: true}, | ||
&InstanceDiff{}, | ||
}, | ||
|
||
// Destroy | ||
{ | ||
&EvalFilterDiff{Destroy: true}, | ||
&InstanceDiff{Destroy: false}, | ||
&InstanceDiff{Destroy: false}, | ||
}, | ||
{ | ||
&EvalFilterDiff{Destroy: true}, | ||
&InstanceDiff{Destroy: true}, | ||
&InstanceDiff{Destroy: true}, | ||
}, | ||
{ | ||
&EvalFilterDiff{Destroy: true}, | ||
&InstanceDiff{ | ||
Destroy: true, | ||
Attributes: map[string]*ResourceAttrDiff{ | ||
"foo": &ResourceAttrDiff{}, | ||
}, | ||
}, | ||
&InstanceDiff{Destroy: true}, | ||
}, | ||
{ | ||
&EvalFilterDiff{Destroy: true}, | ||
&InstanceDiff{ | ||
Attributes: map[string]*ResourceAttrDiff{ | ||
"foo": &ResourceAttrDiff{ | ||
RequiresNew: true, | ||
}, | ||
}, | ||
}, | ||
&InstanceDiff{Destroy: true}, | ||
}, | ||
{ | ||
&EvalFilterDiff{Destroy: true}, | ||
&InstanceDiff{ | ||
Attributes: map[string]*ResourceAttrDiff{ | ||
"foo": &ResourceAttrDiff{}, | ||
}, | ||
}, | ||
&InstanceDiff{Destroy: false}, | ||
}, | ||
} | ||
|
||
for i, tc := range cases { | ||
var output *InstanceDiff | ||
tc.Node.Diff = &tc.Input | ||
tc.Node.Output = &output | ||
if _, err := tc.Node.Eval(ctx); err != nil { | ||
t.Fatalf("err: %s", err) | ||
} | ||
|
||
if !reflect.DeepEqual(output, tc.Output) { | ||
t.Fatalf("bad: %d\n\n%#v", i, output) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mitchellh We've talked about this a bit, and I know you were flying forward on a critical bug here so I'm not talking about this specific instance, but for new test case harnesses can we agree to prefer:
Perhaps I am a dummy, but for tests written in this style it takes me a lot of mental ramp time to be able to jump in and read these coherently. My theory is that a few tweaks today can provide compounding savings of future developer time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I've been thinking about it in each instance, and the main thing for is "does the actual test case provide information vs. what I can provide here" and in this case I figured that the So, I'd say in this case, option 2 is best. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thing is, you don't get input and output... you get something like:
As a future human encountering this, I know I broke something, but how am I to go about figuring out what I broke? If instead, the output was
I immediately know what I broke. I am attempting to optimize for Cost for Future Human Reloading of this Context into their Brains (CFHRCB). 😀 |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,10 +43,9 @@ func (n *EvalReadState) Eval(ctx EvalContext) (interface{}, error) { | |
if idx < 0 { | ||
idx = len(rs.Tainted) - 1 | ||
} | ||
|
||
if idx < len(rs.Tainted) { | ||
if idx >= 0 && idx < len(rs.Tainted) { | ||
// Return the proper tainted resource | ||
result = rs.Tainted[n.TaintedIndex] | ||
result = rs.Tainted[idx] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Err, tests were failing suddenly without this change (core tests). I don't know, I really don't know. |
||
} | ||
} | ||
|
||
|
@@ -58,6 +57,25 @@ func (n *EvalReadState) Eval(ctx EvalContext) (interface{}, error) { | |
return result, nil | ||
} | ||
|
||
// EvalRequireState is an EvalNode implementation that early exits | ||
// if the state doesn't have an ID. | ||
type EvalRequireState struct { | ||
State **InstanceState | ||
} | ||
|
||
func (n *EvalRequireState) Eval(ctx EvalContext) (interface{}, error) { | ||
if n.State == nil { | ||
return nil, EvalEarlyExitError{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: |
||
} | ||
|
||
state := *n.State | ||
if state == nil || state.ID == "" { | ||
return nil, EvalEarlyExitError{} | ||
} | ||
|
||
return nil, nil | ||
} | ||
|
||
// EvalUpdateStateHook is an EvalNode implementation that calls the | ||
// PostStateUpdate hook with the current state. | ||
type EvalUpdateStateHook struct{} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is named to sound like a generic filter, but for that we'd need it to take a function, yeah?
This is really
EvalDestroyStrippingFilter
at this point, if I'm understanding properly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but I expect to expand its feature-set in the future, for now I just did what we needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going over this again, I'm curious as to why there's even a boolean parameter. It's only used in one place:
So it could have no params and just be called
EvalStripEverythingButDestroy
?I guess if you're trying to make these Eval nodes more like generic operations, we could do the function param thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright that's fair. I'll make that change (to make it more specific vs. adding a filter callback, just to keep things simpler)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok gotchya, so it sounds like you're saying its interface will change as we use it in more places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but could just be a premature opt and we can maybe just make more specific eval ndoes.