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

jsonplan: Add replace_paths #28608

Merged
merged 1 commit into from
May 6, 2021
Merged

Conversation

alisdair
Copy link
Contributor

@alisdair alisdair commented May 4, 2021

The set of paths which caused a resource update to require replacement has been stored in the plan since 0.15.0 (#28201). This commit adds a simple JSON representation of these paths, allowing consumers of this format to determine exactly which paths caused the resource to be replaced.

This representation is intentionally more loosely encoded than the JSON state serialization of paths used for sensitive attributes. Instead of a path step being represented by an object with type and value, we use a more-JavaScripty heterogenous array of numbers and strings. Any practical consumer of this format will likely traverse an object tree using the index operator, which should work more easily with this format. It also allows easy prefix comparison for consumers which are tracking paths.

While updating the documentation to include this new field, I noticed that some others were missing, so added them too.

This is the last missing piece of the JSON plan format that I'm aware of, so we might want to bump the output format to 1.0 soon. If so that can be a separate decision from this PR.

The set of paths which caused a resource update to require replacement
has been stored in the plan since 0.15.0 (#28201). This commit adds a
simple JSON representation of these paths, allowing consumers of this
format to determine exactly which paths caused the resource to be
replaced.

This representation is intentionally more loosely encoded than the JSON
state serialization of paths used for sensitive attributes. Instead of a
path step being represented by an object with type and value, we use a
more-JavaScripty heterogenous array of numbers and strings. Any
practical consumer of this format will likely traverse an object tree
using the index operator, which should work more easily with this
format. It also allows easy prefix comparison for consumers which are
tracking paths.

While updating the documentation to include this new field, I noticed
that some others were missing, so added them too.
@alisdair alisdair added cli json-output 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels May 4, 2021
@alisdair alisdair requested review from vancluever and a team May 4, 2021 21:18
@alisdair alisdair self-assigned this May 4, 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.

This looks good to me, and I agree with the rationale for using a simpler JSON-type-system-oriented path serialization.

One of our design decisions for this format was that it was okay to be lossy in our lowering to JSON as long as where necessary it'd be possible to recover the lost information by referring to provider schemas (available from terraform providers schema -json), and that seems to hold here in that a consumer could potentially traverse through the resource type schema at the same time as traversing through the data structure if for some reason they need to decide between indexing or attribute access, because there's no type in cty where both are permitted.

(Context for others potentially viewing this later: HCL's idea of indexing and attribute access is more flexible and treats maps and objects as mostly interchangable, HCL itself deals with that and selects the appropriate operation when calling down into the cty layer, depending on the type of value that's being indexed.)

Copy link
Contributor

@vancluever vancluever left a comment

Choose a reason for hiding this comment

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

LGTM here!

@alisdair alisdair merged commit 91cdde1 into main May 6, 2021
@alisdair alisdair deleted the alisdair/json-plan-replace-paths branch May 6, 2021 12:23
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2021

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

Successfully merging this pull request may close these issues.

3 participants