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

json-output: Add resource drift to machine readable UI #29072

Merged
merged 2 commits into from
Jul 12, 2021

Conversation

alisdair
Copy link
Member

The new resource_drift message allows consumers of the terraform plan -json output to determine which resources Terraform found to have changed externally after the refresh phase.

Note to reviewers: this implementation is a third copy of the logic for determining resource drift for a plan. I'm not happy about this, but I'm struggling to see a way to extract the logic to a single place, given the intertwingled dependencies. Any suggestions?

@alisdair alisdair added json-output 1.0-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Jun 30, 2021
@alisdair alisdair requested a review from a team June 30, 2021 19:08
@alisdair alisdair self-assigned this Jun 30, 2021
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.

Functionality-wise, this looks great to me.

I share your concern but also the uncertainty about how better to factor the logic for taking two states and detecting drift from them. Given the circular-import challenges we had with trying to reuse the types from plans, the best idea I have to hand right now is to establish a new package plans/drift which defines some types similar to some of the model types we have in plans to model changes.

I do wonder about potentially factoring out the change-model-related types into a separate plans/changes package which various other packages could then call on, but that'd be a pretty noisy change in terms of how many users it would update, and also the models in plans are tailored for the specific need of serializing changes into a plan file, and we don't really have those needs here because we can always re-derive the same answer from the pair of state snapshots we already have serialized in there.

Given that, maybe it's not too bad to have some similar-but-not-identical model types specialized for representing drift, particularly if we make an effort to keep them as similar as makes sense to be in case we find a path to converging them later.

🤔

@alisdair
Copy link
Member Author

alisdair commented Jul 9, 2021

I like the plans/drift idea, and would like to pursue that. As I'm still targeting a 1.0 backport for this fix, I won't make that change in this PR, which I'd like to keep as non-invasive as possible.

@alisdair alisdair merged commit 72a7c95 into main Jul 12, 2021
@alisdair alisdair deleted the alisdair/json-ui-resource-drift branch July 13, 2021 15:57
@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 Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.0-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged json-output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants