-
Notifications
You must be signed in to change notification settings - Fork 94
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
tfsdk: Check ProposedNewState for State differences before marking unknowns for Computed-only attributes in plans #184
Conversation
…knowns for Computed-only attributes in plans Reference: #181 Previously, any `Computed` attributes would always trigger plan differences due to the unknown marking. ``` Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: ~ update in-place Terraform will perform the following actions: # hashicups_order.edu will be updated in-place ~ resource "hashicups_order" "edu" { ~ id = "1" -> (known after apply) ~ items = [ ~ { ~ coffee = { + description = (known after apply) id = 3 ~ image = "/nomad.png" -> (known after apply) ~ name = "Nomadicano" -> (known after apply) ~ price = 150 -> (known after apply) ~ teaser = "Drink one today and you will want to schedule another" -> (known after apply) } # (1 unchanged attribute hidden) ~ coffee = { + description = (known after apply) id = 1 ~ image = "/packer.png" -> (known after apply) ~ name = "Packer Spiced Latte" -> (known after apply) ~ price = 350 -> (known after apply) ~ teaser = "Packed with goodness to spice up your images" -> (known after apply) } # (2 unchanged attributes hidden) }, ] ~ last_updated = "Tuesday, 28-Sep-21 11:53:27 EDT" -> (known after apply) } Plan: 0 to add, 1 to change, 0 to destroy. ``` This change limits the unknown marking to only be performed when there is actually a change between the `ProposedNewState` and `State`. Without a configuration change: ``` No changes. Your infrastructure matches the configuration. ``` With a configuration change: ``` Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: ~ update in-place Terraform will perform the following actions: # hashicups_order.edu will be updated in-place ~ resource "hashicups_order" "edu" { ~ id = "1" -> (known after apply) ~ items = [ ~ { ~ coffee = { + description = (known after apply) id = 3 ~ image = "/nomad.png" -> (known after apply) ~ name = "Nomadicano" -> (known after apply) ~ price = 150 -> (known after apply) ~ teaser = "Drink one today and you will want to schedule another" -> (known after apply) } # (1 unchanged attribute hidden) ~ coffee = { + description = (known after apply) ~ id = 1 -> 2 ~ image = "/packer.png" -> (known after apply) ~ name = "Packer Spiced Latte" -> (known after apply) ~ price = 350 -> (known after apply) ~ teaser = "Packed with goodness to spice up your images" -> (known after apply) } # (2 unchanged attributes hidden) }, ] ~ last_updated = "Tuesday, 28-Sep-21 11:53:27 EDT" -> (known after apply) } Plan: 0 to add, 1 to change, 0 to destroy. ``` As a followup, it may be necessary to invoke two passes of attribute and resource plan modifiers to ensure any changes captured by those modifiers triggers the unknown marking. As it stands now, a plan with no configuration changes, but changes due to plan modifiers will not correctly trigger the unknown marking. See also: #183
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.
Some additional breadcrumbs for review, since the real change was just adding && !plan.Equal(state)
@@ -1719,6 +1719,7 @@ func TestServerPlanResourceChange(t *testing.T) { | |||
"favorite_colors": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, []tftypes.Value{ | |||
tftypes.NewValue(tftypes.String, "red"), | |||
tftypes.NewValue(tftypes.String, "orange"), | |||
tftypes.NewValue(tftypes.String, "yellow"), |
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 a test fix since the configuration contained it, so the proposedNewState would also contain it.
@@ -1738,10 +1739,47 @@ func TestServerPlanResourceChange(t *testing.T) { | |||
"favorite_colors": tftypes.NewValue(tftypes.List{ElementType: tftypes.String}, []tftypes.Value{ | |||
tftypes.NewValue(tftypes.String, "red"), | |||
tftypes.NewValue(tftypes.String, "orange"), | |||
tftypes.NewValue(tftypes.String, "yellow"), |
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 a test fix since the proposed new state contained it, so the planned state would also contain it.
@@ -1783,7 +1821,199 @@ func TestServerPlanResourceChange(t *testing.T) { | |||
resourceType: testServeResourceTypeTwoType, | |||
expectedPlannedState: tftypes.NewValue(testServeResourceTypeTwoType, nil), | |||
}, | |||
"three_nested_computed_unknown": { |
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.
NB: This was renamed to three_nested_computed_nested_configuration_change
as seen below the next Git diff, just to capture the additional nuance between the two new test cases and the original.
@@ -92,6 +92,10 @@ func (rt testServeResourceTypeAttributePlanModifiers) GetSchema(_ context.Contex | |||
Type: types.StringType, | |||
PlanModifiers: []AttributePlanModifier{testAttrDefaultValueModifier{}}, | |||
}, | |||
"computed_string_no_modifiers": { |
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.
Additional attribute on this resource to allow us to further verify plan unknown marking, since this resource has attribute plan modifiers and allowed me to catch #183 for the future. It does not have to be part of this pull request though. 😄
Getting a slightly unexpected result with this. Configuration: provider "hashicups" {
username = "education"
password = "test123"
host = "http://localhost:19090"
}
resource "hashicups_order" "edu" {
items = [{
coffee = {
id = 3
}
quantity = 2
}, {
coffee = {
id = 2
}
quantity = 2
}
]
} Steps
ResultThe output from the second apply is as in the PR description, but when actually running the apply we get this:
Edit: This error on apply also happens with Edit 2: This appears to be a bug in the hashicups provider or server. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Closes #181
Reference: https://github.com/hashicorp/terraform/blob/main/docs/resource-instance-change-lifecycle.md
ProposedNewState
is the mergedPriorState
andConfiguration
. This allows us to verify if any configuration changes have occurred.Previously, any
Computed
attributes would always trigger plan differences due to the unknown marking.This change limits the unknown marking to only be performed when there is actually a change between the
ProposedNewState
andState
.Without a configuration change:
With a configuration change:
As a followup, it may be necessary to invoke two passes of attribute and resource plan modifiers to ensure any changes captured by those modifiers triggers the unknown marking. As it stands now, a plan with no configuration changes, but changes due to plan modifiers will not correctly trigger the unknown marking. See also: #183