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

filter computed attrs from ignore_changes=all #31747

Merged
merged 2 commits into from Sep 28, 2022

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Sep 6, 2022

When handling ignore_changes=all, we must filter computed attributes
from the prior state to prevent them showing in the configuration. Since
it's not valid for the user to have set computed attributes in the
config, the provider should expect to never see any values there. The
oversight has only now become apparent, as more providers adopt the
plugin-framework which has direct access to the plan-time configuration
value.

When handling ignore_changes=all, we must filter computed attributes
from the prior state to prevent them showing in the configuration. Since
it's not valid for the user to have set computed attributes in the
config, the provider should expect to never see any values there. The
oversight has only now become apparent, as more providers adopt the
plugin-framework which has direct access to the plan-time configuration
value.
@jbardin jbardin added the 1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Sep 6, 2022
@jbardin jbardin requested a review from a team September 6, 2022 20:33
@jbardin jbardin force-pushed the jbardin/ignore-changes-all-computed branch from 112eb04 to 3e281e1 Compare September 20, 2022 15:00
@jbardin jbardin requested a review from kmoe September 23, 2022 18:56
@jbardin jbardin self-assigned this Sep 23, 2022
@jbardin jbardin requested review from a team and removed request for kmoe September 28, 2022 13:02
Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Looks good to me. Is there an associated issue? I thought I saw something recently that seemed related but I can't find it now.

@jbardin
Copy link
Member Author

jbardin commented Sep 28, 2022

This was reported internally by the SDK team, I don't think there was a specific GH issue.

@jbardin jbardin merged commit 8c98e1f into main Sep 28, 2022
@jbardin jbardin deleted the jbardin/ignore-changes-all-computed branch September 28, 2022 13:27
@github-actions
Copy link

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

@bflad
Copy link
Member

bflad commented Sep 28, 2022

This might be the associated issue you are looking for: hashicorp/terraform-provider-random#302 Similarly down in the comments, hashicorp/terraform-provider-random#325.

@github-actions
Copy link

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 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants