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

internal/fwserver: Use existing data instead of path-based lookups during plan modification #468

Merged
merged 5 commits into from Sep 8, 2022

Conversation

bflad
Copy link
Member

@bflad bflad commented Sep 7, 2022

Closes #293

This change is a major rewrite of the plan modification logic to move away from globally mutating the schema response to instead localize handling of each attribute/block. The logic still transverses downward in the schema while updated plan modifier values are passed up the call stack to eventually overwrite the top level attribute/block data. The main benefit of this logic is that it cannot lose track of which element to update within a set and may have some nominal performance improvements.

The majority of the logic changes are convincing the plan modification process that nested attributes and blocks have specific types for their data, which allows it to safely traverse deeper based on the schema definition. There is just a lot of repeated diagnostics handling.

The new TestAttributeModifyPlan/attribute-set-nested-usestateforunknown and TestBlockModifyPlan/block-set-nested-usestateforunknown unit test cases specifically test a similar issue to the linked one where sets previously could not properly use resource.UseStateForKnown() for set nested attributes or set blocks.

…ring plan modification

Reference: #293

This change is a major rewrite of the plan modification logic to move away from globally mutating the schema response to instead localize handling of each attribute/block. The logic still transverses downward in the schema while updated plan modifier values are passed up the call stack to eventually overwrite the top level attribute/block data. The main benefit of this logic is that it cannot lose track of which element to update within a set and may have some nominal performance improvements.

The majority of the logic changes are convincing the plan modification process that nested attributes and blocks have specific types for their data, which allows it to safely traverse deeper based on the schema definition. There is just a lot of repeated diagnostics handling.
@bflad bflad added the bug Something isn't working label Sep 7, 2022
@bflad
Copy link
Member Author

bflad commented Sep 7, 2022

Taking a fresh look at this PR this morning, I think this is something we do want to do for now. I'm really sorry for the awful Git differences with the testing code, but essentially the changes were to replace Config/Plan/State fields with AttributeConfig/AttributePlan/AttributeState and remove any test cases dealing with "previous errors". The "previous errors" testing was there before because all response diagnostics were passed to all plan modifiers before. We don't have a known use case for that behavior and even if we did, the behavior would be completely inconsistent as attribute plan modifiers run in arbitrary ordering (not something we likely want to fix/adjust).

@bflad bflad marked this pull request as ready for review September 7, 2022 14:22
@bflad bflad requested a review from a team as a code owner September 7, 2022 14:22
Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

LGTM

internal/fwserver/attr_value.go Outdated Show resolved Hide resolved
bflad and others added 2 commits September 7, 2022 14:27
@github-actions
Copy link

github-actions bot commented Oct 9, 2022

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 Oct 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with Nested Set Attributes with PlanModifiers
2 participants