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

Hide empty plans for misbehaving data resource #25302

Merged
merged 1 commit into from
Jun 22, 2020

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jun 18, 2020

If a data source is storing a value that doesn't comply precisely with
the schema, it will now show up as a perpetual diff during plan. We
don't have a strict check for this, as data sources do not have the
LegacyTypeSystem flag to allow this behavior, but we can use the
same verification we use for managed resources.

Since we can easily detect if there is no resulting change from the
stored value, rather than presenting a planned read each time, we can
change the plan to a NoOp and log the incongruity as a warning.

Fixes #25179, at least from the core side. Any other unexplained plans
would need to be fixed in the provider.

If a data source is storing a value that doesn't comply precisely with
the schema, it will now show up as a perpetual diff during plan.

Since we can easily detect if there is no resulting change from the
stored value, rather than presenting a planned read each time, we can
change the plan to a NoOp and log the incongruity as a warning.
@jbardin jbardin requested a review from a team June 18, 2020 23:22
@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #25302 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
terraform/eval_read_data_plan.go 84.21% <100.00%> (+2.50%) ⬆️
dag/walk.go 93.16% <0.00%> (+0.62%) ⬆️
terraform/node_resource_plan.go 93.44% <0.00%> (+1.63%) ⬆️
communicator/communicator.go 83.33% <0.00%> (+3.70%) ⬆️

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This makes sense to me!

It seems like there isn't really any reasonable path to upgrade this from a warning to an error because adding a new opt-out flag like we did for resources would fail for providers that don't know how to set it, and adding an "opt in to Terraform being stricter" is not a useful design either. That's a shame, but I don't have any other ideas to suggest, so I guess we'll just need to keep an eye out for opportunities to make other breaking changes to this particular provider API in future and bundle stricter error checking along with it.

@jbardin jbardin merged commit 06493d7 into master Jun 22, 2020
@jbardin jbardin deleted the jbardin/data-source-plan branch June 22, 2020 16:04
@ghost
Copy link

ghost commented Jul 23, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jul 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

data resource read is deferred to apply phase in v0.13, but wasn't in v0.12
2 participants