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

Implement schema-aware diffing for detail diff #2294

Closed
t0yv0 opened this issue Aug 9, 2024 · 1 comment · Fixed by #2405
Closed

Implement schema-aware diffing for detail diff #2294

t0yv0 opened this issue Aug 9, 2024 · 1 comment · Fixed by #2405
Assignees
Labels
kind/task Work that's part of an ongoing epic resolution/fixed This issue was fixed
Milestone

Comments

@t0yv0
Copy link
Member

t0yv0 commented Aug 9, 2024

The task is to build an algo that compares prior and planned state at Pulumi level.

Resolves #1895
Resolves #186
Resolves #1504

func makePulumiDetailedDiff(
	ctx context.Context,
	tfs shim.SchemaMap,
	ps map[string]*SchemaInfo,
	oldState, plannedState resource.PropertyMap,
) (map[string]*pulumirpc.PropertyDiff) {

This is intended to replace the current signature that tries to do inference on the shim.InstanceDiff object that wraps the terraform.InstanceDiff object:

func makeDetailedDiff(
	ctx context.Context,
	tfs shim.SchemaMap,
	ps map[string]*SchemaInfo,
	olds, news resource.PropertyMap,
	tfDiff shim.InstanceDiff,
) (map[string]*pulumirpc.PropertyDiff, pulumirpc.DiffResponse_DiffChanges) {

As TF CLI does not access terraform.InstanceDiff but renders diffs by comparing existing and planned state, this would make bridged providers behave more in line with TF expected behavior.

The schema-aware requirement comes down to keep tracking schema (SchemaInfo/SchemaMap) or else current paths through the transformation. When diffing collections, the algo needs to be able to tell when the two diffed PropertyValue arrays are representing a TF set under the hood. PropertyPathToSchemaPath and LookupSchemas can be helpful here.

#2200 outlines a suggestion for how set collection diffs should be handled. The cases where plannedState has unknowns with sets are a bit under-specified, suggest treating the entire new set as an unknown in that case old => UNKNOWN.

This change should be flagged together with all the other work in the epic for a consistent rollout.

@pulumi-bot
Copy link
Contributor

This issue has been addressed in PR #2405 and shipped in release v3.92.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Work that's part of an ongoing epic resolution/fixed This issue was fixed
Projects
None yet
4 participants