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.

Sign up for GitHub

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

Jump to bottom

Extra feedback for a variety of "move" consequences and edge-cases #29637

Merged
merged 5 commits into from
Sep 23, 2021
Merged

Extra feedback for a variety of "move" consequences and edge-cases #29637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
56ab5d8
refactoring: Don't implicitly move for resources with for_each
apparentlymart Sep 22, 2021
22412de
core: Report ActionReasons when we plan to delete "orphans"
apparentlymart Sep 23, 2021
c563586
command/format: Include deletion reasons in plan report
apparentlymart Sep 23, 2021
b417aa8
core: Report a warning if any moves get blocked
apparentlymart Sep 23, 2021
f371f46
core: Don't return other errors if move statements are invalid
apparentlymart Sep 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 26 additions & 1 deletion internal/command/format/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,31 @@ func ResourceChange(
default:
buf.WriteString(fmt.Sprintf(color.Color("[bold] # %s[reset] delete (unknown reason %s)"), dispAddr, language))
}
// We can sometimes give some additional detail about why we're
// proposing to delete. We show this as additional notes, rather than
// as additional wording in the main action statement, in an attempt
// to make the "will be destroyed" message prominent and consistent
// in all cases, for easier scanning of this often-risky action.
switch change.ActionReason {
case plans.ResourceInstanceDeleteBecauseNoResourceConfig:
buf.WriteString(fmt.Sprintf("\n # (because %s is not in configuration)", addr.Resource.Resource))
case plans.ResourceInstanceDeleteBecauseNoModule:
buf.WriteString(fmt.Sprintf("\n # (because %s is not in configuration)", addr.Module))
case plans.ResourceInstanceDeleteBecauseWrongRepetition:
// We have some different variations of this one
switch addr.Resource.Key.(type) {
case nil:
buf.WriteString("\n # (because resource uses count or for_each)")
case addrs.IntKey:
buf.WriteString("\n # (because resource does not use count)")
case addrs.StringKey:
buf.WriteString("\n # (because resource does not use for_each)")
}
case plans.ResourceInstanceDeleteBecauseCountIndex:
buf.WriteString(fmt.Sprintf("\n # (because index %s is out of range for count)", addr.Resource.Key))
case plans.ResourceInstanceDeleteBecauseEachKey:
buf.WriteString(fmt.Sprintf("\n # (because key %s is not in for_each map)", addr.Resource.Key))
}
if change.DeposedKey != states.NotDeposed {
// Some extra context about this unusual situation.
buf.WriteString(color.Color("\n # (left over from a partially-failed replacement of this instance)"))
Expand All @@ -115,7 +140,7 @@ func ResourceChange(
buf.WriteString(color.Color("[reset]\n"))

if change.Moved() && change.Action != plans.NoOp {
buf.WriteString(fmt.Sprintf(color.Color("[bold] # [reset]([bold]%s[reset] has moved to [bold]%s[reset])\n"), change.PrevRunAddr.String(), dispAddr))
buf.WriteString(fmt.Sprintf(color.Color(" # [reset](moved from %s)\n"), change.PrevRunAddr.String()))
}

if change.Moved() && change.Action == plans.NoOp {
Expand Down
229 changes: 227 additions & 2 deletions internal/command/format/diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3504,6 +3504,229 @@ func TestResourceChange_nestedMap(t *testing.T) {
runTestCases(t, testCases)
}

func TestResourceChange_actionReason(t *testing.T) {
emptySchema := &configschema.Block{}
nullVal := cty.NullVal(cty.EmptyObject)
emptyVal := cty.EmptyObjectVal

testCases := map[string]testCase{
"delete for no particular reason": {
Action: plans.Delete,
ActionReason: plans.ResourceInstanceChangeNoReason,
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example will be destroyed
- resource "test_instance" "example" {}
`,
},
"delete because of wrong repetition mode (NoKey)": {
Action: plans.Delete,
ActionReason: plans.ResourceInstanceDeleteBecauseWrongRepetition,
Mode: addrs.ManagedResourceMode,
InstanceKey: addrs.NoKey,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example will be destroyed
# (because resource uses count or for_each)
- resource "test_instance" "example" {}
`,
},
"delete because of wrong repetition mode (IntKey)": {
Action: plans.Delete,
ActionReason: plans.ResourceInstanceDeleteBecauseWrongRepetition,
Mode: addrs.ManagedResourceMode,
InstanceKey: addrs.IntKey(1),
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example[1] will be destroyed
# (because resource does not use count)
- resource "test_instance" "example" {}
`,
},
"delete because of wrong repetition mode (StringKey)": {
Action: plans.Delete,
ActionReason: plans.ResourceInstanceDeleteBecauseWrongRepetition,
Mode: addrs.ManagedResourceMode,
InstanceKey: addrs.StringKey("a"),
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example["a"] will be destroyed
# (because resource does not use for_each)
- resource "test_instance" "example" {}
`,
},
"delete because no resource configuration": {
Action: plans.Delete,
ActionReason: plans.ResourceInstanceDeleteBecauseNoResourceConfig,
ModuleInst: addrs.RootModuleInstance.Child("foo", addrs.NoKey),
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # module.foo.test_instance.example will be destroyed
# (because test_instance.example is not in configuration)
- resource "test_instance" "example" {}
`,
},
"delete because no module": {
Action: plans.Delete,
ActionReason: plans.ResourceInstanceDeleteBecauseNoModule,
ModuleInst: addrs.RootModuleInstance.Child("foo", addrs.IntKey(1)),
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # module.foo[1].test_instance.example will be destroyed
# (because module.foo[1] is not in configuration)
- resource "test_instance" "example" {}
`,
},
"delete because out of range for count": {
Action: plans.Delete,
ActionReason: plans.ResourceInstanceDeleteBecauseCountIndex,
Mode: addrs.ManagedResourceMode,
InstanceKey: addrs.IntKey(1),
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example[1] will be destroyed
# (because index [1] is out of range for count)
- resource "test_instance" "example" {}
`,
},
"delete because out of range for for_each": {
Action: plans.Delete,
ActionReason: plans.ResourceInstanceDeleteBecauseEachKey,
Mode: addrs.ManagedResourceMode,
InstanceKey: addrs.StringKey("boop"),
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example["boop"] will be destroyed
# (because key ["boop"] is not in for_each map)
- resource "test_instance" "example" {}
`,
},
"replace for no particular reason (delete first)": {
Action: plans.DeleteThenCreate,
ActionReason: plans.ResourceInstanceChangeNoReason,
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example must be replaced
-/+ resource "test_instance" "example" {}
`,
},
"replace for no particular reason (create first)": {
Action: plans.CreateThenDelete,
ActionReason: plans.ResourceInstanceChangeNoReason,
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example must be replaced
+/- resource "test_instance" "example" {}
`,
},
"replace by request (delete first)": {
Action: plans.DeleteThenCreate,
ActionReason: plans.ResourceInstanceReplaceByRequest,
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example will be replaced, as requested
-/+ resource "test_instance" "example" {}
`,
},
"replace by request (create first)": {
Action: plans.CreateThenDelete,
ActionReason: plans.ResourceInstanceReplaceByRequest,
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example will be replaced, as requested
+/- resource "test_instance" "example" {}
`,
},
"replace because tainted (delete first)": {
Action: plans.DeleteThenCreate,
ActionReason: plans.ResourceInstanceReplaceBecauseTainted,
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example is tainted, so must be replaced
-/+ resource "test_instance" "example" {}
`,
},
"replace because tainted (create first)": {
Action: plans.CreateThenDelete,
ActionReason: plans.ResourceInstanceReplaceBecauseTainted,
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example is tainted, so must be replaced
+/- resource "test_instance" "example" {}
`,
},
"replace because cannot update (delete first)": {
Action: plans.DeleteThenCreate,
ActionReason: plans.ResourceInstanceReplaceBecauseCannotUpdate,
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
// This one has no special message, because the fuller explanation
// typically appears inline as a "# forces replacement" comment.
// (not shown here)
ExpectedOutput: ` # test_instance.example must be replaced
-/+ resource "test_instance" "example" {}
`,
},
"replace because cannot update (create first)": {
Action: plans.CreateThenDelete,
ActionReason: plans.ResourceInstanceReplaceBecauseCannotUpdate,
Mode: addrs.ManagedResourceMode,
Before: emptyVal,
After: nullVal,
Schema: emptySchema,
RequiredReplace: cty.NewPathSet(),
// This one has no special message, because the fuller explanation
// typically appears inline as a "# forces replacement" comment.
// (not shown here)
ExpectedOutput: ` # test_instance.example must be replaced
+/- resource "test_instance" "example" {}
`,
},
}

runTestCases(t, testCases)
}

func TestResourceChange_sensitiveVariable(t *testing.T) {
testCases := map[string]testCase{
"creation": {
Expand Down Expand Up @@ -4479,7 +4702,7 @@ func TestResourceChange_moved(t *testing.T) {
},
RequiredReplace: cty.NewPathSet(),
ExpectedOutput: ` # test_instance.example will be updated in-place
# (test_instance.previous has moved to test_instance.example)
# (moved from test_instance.previous)
~ resource "test_instance" "example" {
~ bar = "baz" -> "boop"
id = "12345"
Expand Down Expand Up @@ -4524,7 +4747,9 @@ func TestResourceChange_moved(t *testing.T) {
type testCase struct {
Action plans.Action
ActionReason plans.ResourceInstanceChangeActionReason
ModuleInst addrs.ModuleInstance
Mode addrs.ResourceMode
InstanceKey addrs.InstanceKey
DeposedKey states.DeposedKey
Before cty.Value
BeforeValMarks []cty.PathValueMarks
Expand Down Expand Up @@ -4571,7 +4796,7 @@ func runTestCases(t *testing.T, testCases map[string]testCase) {
Mode: tc.Mode,
Type: "test_instance",
Name: "example",
}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance)
}.Instance(tc.InstanceKey).Absolute(tc.ModuleInst)

prevRunAddr := tc.PrevRunAddr
// If no previous run address is given, reuse the current address
Expand Down
10 changes: 10 additions & 0 deletions internal/command/jsonplan/plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,16 @@ func (p *plan) marshalResourceChanges(resources []*plans.ResourceInstanceChangeS
r.ActionReason = "replace_because_tainted"
case plans.ResourceInstanceReplaceByRequest:
r.ActionReason = "replace_by_request"
case plans.ResourceInstanceDeleteBecauseNoResourceConfig:
r.ActionReason = "delete_because_no_resource_config"
case plans.ResourceInstanceDeleteBecauseWrongRepetition:
r.ActionReason = "delete_because_wrong_repetition"
case plans.ResourceInstanceDeleteBecauseCountIndex:
r.ActionReason = "delete_because_count_index"
case plans.ResourceInstanceDeleteBecauseEachKey:
r.ActionReason = "delete_because_each_key"
case plans.ResourceInstanceDeleteBecauseNoModule:
r.ActionReason = "delete_because_no_module"
default:
return nil, fmt.Errorf("resource %s has an unsupported action reason %s", r.Address, rc.ActionReason)
}
Expand Down
1 change: 1 addition & 0 deletions internal/command/testdata/show-json/basic-delete/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"type": "test_instance",
"provider_name": "registry.terraform.io/hashicorp/test",
"name": "test-delete",
"action_reason": "delete_because_no_resource_config",
"change": {
"actions": [
"delete"
Expand Down
34 changes: 34 additions & 0 deletions internal/plans/changes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,40 @@ const (
// the ResourceInstanceChange object to give information about specifically
// which arguments changed in a non-updatable way.
ResourceInstanceReplaceBecauseCannotUpdate ResourceInstanceChangeActionReason = 'F'

// ResourceInstanceDeleteBecauseNoResourceConfig indicates that the
// resource instance is planned to be deleted because there's no
// corresponding resource configuration block in the configuration.
ResourceInstanceDeleteBecauseNoResourceConfig ResourceInstanceChangeActionReason = 'N'

// ResourceInstanceDeleteBecauseWrongRepetition indicates that the
// resource instance is planned to be deleted because the instance key
// type isn't consistent with the repetition mode selected in the
// resource configuration.
ResourceInstanceDeleteBecauseWrongRepetition ResourceInstanceChangeActionReason = 'W'

// ResourceInstanceDeleteBecauseCountIndex indicates that the resource
// instance is planned to be deleted because its integer instance key
// is out of range for the current configured resource "count" value.
ResourceInstanceDeleteBecauseCountIndex ResourceInstanceChangeActionReason = 'C'

// ResourceInstanceDeleteBecauseEachKey indicates that the resource
// instance is planned to be deleted because its string instance key
// isn't one of the keys included in the current configured resource
// "for_each" value.
ResourceInstanceDeleteBecauseEachKey ResourceInstanceChangeActionReason = 'E'

// ResourceInstanceDeleteBecauseNoModule indicates that the resource
// instance is planned to be deleted because it belongs to a module
// instance that's no longer declared in the configuration.
//
// This is less specific than the reasons we return for the various ways
// a resource instance itself can be no longer declared, including both
// the total removal of a module block and changes to its count/for_each
// arguments. This difference in detail is out of pragmatism, because
// potentially multiple nested modules could all contribute conflicting
// specific reasons for a particular instance to no longer be declared.
ResourceInstanceDeleteBecauseNoModule ResourceInstanceChangeActionReason = 'M'
Copy link
Member

Choose a reason for hiding this comment

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

When adding new reasons here, I think we should also update the streaming JSON UI output to match:

terraform/internal/command/views/json/change.go

Lines 70 to 94 in c8cd0b1

const (
ReasonNone ChangeReason = ""
ReasonTainted ChangeReason = "tainted"
ReasonRequested ChangeReason = "requested"
ReasonCannotUpdate ChangeReason = "cannot_update"
ReasonUnknown ChangeReason = "unknown"
)
func changeReason(reason plans.ResourceInstanceChangeActionReason) ChangeReason {
switch reason {
case plans.ResourceInstanceChangeNoReason:
return ReasonNone
case plans.ResourceInstanceReplaceBecauseTainted:
return ReasonTainted
case plans.ResourceInstanceReplaceByRequest:
return ReasonRequested
case plans.ResourceInstanceReplaceBecauseCannotUpdate:
return ReasonCannotUpdate
default:
// This should never happen, but there's no good way to guarantee
// exhaustive handling of the enum, so a generic fall back is better
// than a misleading result or a panic
return ReasonUnknown
}
}

I'll submit a PR for these changes shortly. Can you think of any automated way to keep these two enums in sync for future changes? Maybe adding nishanths/exhaustive as a CI check?

Copy link
Member

Choose a reason for hiding this comment

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

#29649

)

// OutputChange describes a change to an output value.
Expand Down