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

states: check root output value changes in refresh-only mode #35812

Conversation

bschaatsbergen
Copy link
Member

@bschaatsbergen bschaatsbergen commented Oct 3, 2024

Fixes #35806

In refresh-only mode, we don’t expect to propose any actions, but a plan is marked as “applyable” if there are state changes between runs. Currently, a plan is considered “applyable” only when there are differences in managed resources, which overlooks changes in root output values. As a result, a plan is marked as non-applyable even when the root output values have changed, since only managed resources are being checked.

The states package has an Equal function that checks if two states are deeply equal. While this could solve the issue, it seemed like an overreach, as it focuses on managed resource instances only. To address this, I added a function specifically to detect changes in root output values between states.

It’s also worth noting that output values in other modules don’t persist between runs. Terraform Core tracks these internally, but they aren’t included in any artifacts that persist across executions.

Question for the reviewer: What about ephemeral output values? And, are data sources considered as part of the existing ManagedResourcesEqual function already?

 $ go test -run "TestState" -v
=== RUN   TestState
--- PASS: TestState (0.00s)
=== RUN   TestStateDeepCopyObject
--- PASS: TestStateDeepCopyObject (0.00s)
=== RUN   TestStateDeepCopy
--- PASS: TestStateDeepCopy (0.00s)
=== RUN   TestStateHasResourceInstanceObjects
=== RUN   TestStateHasResourceInstanceObjects/empty
=== RUN   TestStateHasResourceInstanceObjects/one_current,_ready_object_in_root_module
=== RUN   TestStateHasResourceInstanceObjects/one_current,_ready_object_in_child_module
=== RUN   TestStateHasResourceInstanceObjects/one_current,_tainted_object_in_root_module
=== RUN   TestStateHasResourceInstanceObjects/one_deposed,_ready_object_in_root_module
=== RUN   TestStateHasResourceInstanceObjects/one_empty_resource_husk_in_root_module
=== RUN   TestStateHasResourceInstanceObjects/one_current_data_resource_object_in_root_module
--- PASS: TestStateHasResourceInstanceObjects (0.00s)
    --- PASS: TestStateHasResourceInstanceObjects/empty (0.00s)
    --- PASS: TestStateHasResourceInstanceObjects/one_current,_ready_object_in_root_module (0.00s)
    --- PASS: TestStateHasResourceInstanceObjects/one_current,_ready_object_in_child_module (0.00s)
    --- PASS: TestStateHasResourceInstanceObjects/one_current,_tainted_object_in_root_module (0.00s)
    --- PASS: TestStateHasResourceInstanceObjects/one_deposed,_ready_object_in_root_module (0.00s)
    --- PASS: TestStateHasResourceInstanceObjects/one_empty_resource_husk_in_root_module (0.00s)
    --- PASS: TestStateHasResourceInstanceObjects/one_current_data_resource_object_in_root_module (0.00s)
=== RUN   TestStateHasRootOutputValues
=== RUN   TestStateHasRootOutputValues/empty
=== RUN   TestStateHasRootOutputValues/one_output_value
=== RUN   TestStateHasRootOutputValues/one_secret_output_value
=== RUN   TestStateHasRootOutputValues/one_output_value_in_child_module
=== RUN   TestStateHasRootOutputValues/one_output_value_in_multi_module
--- PASS: TestStateHasRootOutputValues (0.00s)
    --- PASS: TestStateHasRootOutputValues/empty (0.00s)
    --- PASS: TestStateHasRootOutputValues/one_output_value (0.00s)
    --- PASS: TestStateHasRootOutputValues/one_secret_output_value (0.00s)
    --- PASS: TestStateHasRootOutputValues/one_output_value_in_child_module (0.00s)
    --- PASS: TestStateHasRootOutputValues/one_output_value_in_multi_module (0.00s)
=== RUN   TestState_MoveAbsResource
=== RUN   TestState_MoveAbsResource/basic_move
=== RUN   TestState_MoveAbsResource/move_to_new_module
=== RUN   TestState_MoveAbsResource/from_a_child_module_to_root
=== RUN   TestState_MoveAbsResource/module_to_new_module
=== RUN   TestState_MoveAbsResource/module_to_new_module#01
--- PASS: TestState_MoveAbsResource (0.00s)
    --- PASS: TestState_MoveAbsResource/basic_move (0.00s)
    --- PASS: TestState_MoveAbsResource/move_to_new_module (0.00s)
    --- PASS: TestState_MoveAbsResource/from_a_child_module_to_root (0.00s)
    --- PASS: TestState_MoveAbsResource/module_to_new_module (0.00s)
    --- PASS: TestState_MoveAbsResource/module_to_new_module#01 (0.00s)
=== RUN   TestState_MaybeMoveAbsResource
=== RUN   TestState_MaybeMoveAbsResource/first_move
=== RUN   TestState_MaybeMoveAbsResource/noop
--- PASS: TestState_MaybeMoveAbsResource (0.00s)
    --- PASS: TestState_MaybeMoveAbsResource/first_move (0.00s)
    --- PASS: TestState_MaybeMoveAbsResource/noop (0.00s)
=== RUN   TestState_MoveAbsResourceInstance
=== RUN   TestState_MoveAbsResourceInstance/resource_to_resource_instance
=== RUN   TestState_MoveAbsResourceInstance/move_to_new_module
--- PASS: TestState_MoveAbsResourceInstance (0.00s)
    --- PASS: TestState_MoveAbsResourceInstance/resource_to_resource_instance (0.00s)
    --- PASS: TestState_MoveAbsResourceInstance/move_to_new_module (0.00s)
=== RUN   TestState_MaybeMoveAbsResourceInstance
=== RUN   TestState_MaybeMoveAbsResourceInstance/first_move
=== RUN   TestState_MaybeMoveAbsResourceInstance/noop
--- PASS: TestState_MaybeMoveAbsResourceInstance (0.00s)
    --- PASS: TestState_MaybeMoveAbsResourceInstance/first_move (0.00s)
    --- PASS: TestState_MaybeMoveAbsResourceInstance/noop (0.00s)
=== RUN   TestState_MoveModuleInstance
--- PASS: TestState_MoveModuleInstance (0.00s)
=== RUN   TestState_MaybeMoveModuleInstance
=== RUN   TestState_MaybeMoveModuleInstance/first_move
=== RUN   TestState_MaybeMoveModuleInstance/noop
--- PASS: TestState_MaybeMoveModuleInstance (0.00s)
    --- PASS: TestState_MaybeMoveModuleInstance/first_move (0.00s)
    --- PASS: TestState_MaybeMoveModuleInstance/noop (0.00s)
=== RUN   TestState_MoveModule
=== RUN   TestState_MoveModule/basic
=== RUN   TestState_MoveModule/nested_modules
--- PASS: TestState_MoveModule (0.00s)
    --- PASS: TestState_MoveModule/basic (0.00s)
    --- PASS: TestState_MoveModule/nested_modules (0.00s)
PASS
ok      github.com/hashicorp/terraform/internal/states  0.298s

@bschaatsbergen bschaatsbergen marked this pull request as draft October 3, 2024 23:43
@bschaatsbergen bschaatsbergen marked this pull request as ready for review October 3, 2024 23:44
…sh mode

In refresh-only mode, we do not anticipate proposing any actions; however, a plan is marked as “applyable” if there are changes in the state between runs. Currently, a plan is considered “applyable” only when there are differences in managed resources. This approach seems to overlook changes in root output values. As a result, a plan can be marked as non-applyable, even when there are changes in the root output value, due to the lack of detected changes since only managed resources were checked.
@bschaatsbergen bschaatsbergen force-pushed the b/eval-output-values-in-refresh-mode branch from b5e65c7 to c270e60 Compare October 4, 2024 00:00
@crw crw added the bug label Oct 11, 2024
@liamcervante
Copy link
Member

I think I'm okay with this change. Can you get all the tests passing @bschaatsbergen?

@radeksimko
Copy link
Member

Question for the reviewer: What about ephemeral output values?

We decided to reject root ephemeral output values as per #35791 so they will not be playing any role in the plan. Ephemeral outputs may only be used internally between nested modules.

And, are data sources considered as part of the existing ManagedResourcesEqual function already?

The name would suggest they are not but I'm not aware of any particular issues with data sources during terraform apply -refresh-only currently, are you?

@jbardin
Copy link
Member

jbardin commented Oct 17, 2024

No, data sources are not considered in the comparison, they only really exist in the state at all for historical reasons and don't need to be refreshed.

@bschaatsbergen
Copy link
Member Author

I think I'm okay with this change. Can you get all the tests passing @bschaatsbergen?

Fixed, 73320c5

@liamcervante liamcervante merged commit 098234e into hashicorp:main Oct 23, 2024
5 of 6 checks passed
Copy link
Contributor

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@bschaatsbergen bschaatsbergen deleted the b/eval-output-values-in-refresh-mode branch October 25, 2024 15:19
Copy link
Contributor

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

terraform apply -refresh-only results in Error: Cannot apply non-applyable plan
5 participants