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

Don't display plan diffs with only data reads #12432

Closed
wants to merge 1 commit into from

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Mar 3, 2017

If the only action in a plan is to read data, possibly because the data
source depends on a value to be computed, we don't need to display that
in the plan output.

The add change and destroy counts must also be 0 to supress output.
These should match the plan diff, but check them as an added failsafe.

Possible fix for issues like #11806?

If the only action in a plan is to read data, possibly because the data
source depends on a value to be computed, we don't need to display that
in the plan output.

The add change and destroy counts must also be 0 to supress output.
These should match the plan diff, but check them as an added failsafe.
@jbardin jbardin force-pushed the jbardin/depends_on branch from 5796259 to 8e22323 Compare March 3, 2017 23:41
@josephholsten
Copy link
Contributor

josephholsten commented Mar 4, 2017

We're assuming users are comfortable just using terraform state show for seeing what data has changed? Of course, any data that actually affects resources will be rendered. Seems fine by me.

@jbardin
Copy link
Member Author

jbardin commented Mar 6, 2017

This actually only suppresses output with no changes, since it's just indicating there will be another read of the data source. While this looks valid on its own, and fixes the sample in the linked issue, I'm not certain this is that useful. Anything that depends on the data source will still be transitively marked as unknown, which is much more common in real configurations. I pushed this as a possible easy fix for this particular case, but maybe it's not needed if we can determine a way to remove this change from the graph altogether.

@apparentlymart
Copy link
Contributor

@jbardin the usual symptom of the problem is that the data resource is used in a value for a ForceNew attribute and so Terraform believes it needs to replace the resource on each run.

This change wouldn't address that since it is just a cosmetic update to the UI.

The main UI issue I've noted here is that users don't generally understand that when a data read appears in a plan this means that there is some dependency that needs to be resolved. It would perhaps be helpful to try to describe the reason why a data source read is deferred to the apply phase so that users can understand what they need to do in order to avoid that; right now we give no feedback at all that a delayed read is an unusual case that can be avoided.

@jbardin
Copy link
Member Author

jbardin commented Mar 6, 2017

@apparentlymart,

Yes, this was just a cosmetic update (also changing the return code to go along with empty output). Maybe there is some way we can better document "are you sure you want depends_on here?".

This wasn't intended to replace a possible fix where we can determine that there is actually no change.

@apparentlymart
Copy link
Contributor

I think having a note under the diff when it is caused by depends_on could be a good temp fix here, but unfortunately I don't think the diff renderer has enough info to do that right now. :(

@gechr
Copy link
Contributor

gechr commented May 15, 2017

Though not directly related to this PR, any chance we can bundle diff-related PR #11081 into whatever release this makes it into?

@jbardin jbardin closed this Sep 7, 2017
@jbardin jbardin deleted the jbardin/depends_on branch September 7, 2017 15:10
@ghost
Copy link

ghost commented Apr 7, 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 Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants