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

Consider simplifying Diff implementation for legacy provider interface #1245

Closed
lukehoban opened this issue Jun 26, 2023 · 3 comments
Closed
Labels
kind/engineering Work that is not visible to an external user resolution/wont-fix This issue won't be fixed

Comments

@lukehoban
Copy link
Contributor

From #1243:

This code serves two purposes:

  • implementing ignoreChanges feature
  • computing DetailedDiff
    From digging around earlier, it appeared that DetailedDiff is an optional feature, and if let unimplemented, the engine computes and displays an adequate diff anyway including for nested values. Based on this, Plugin Framework providers still leave it as unimplemented. It this accurate or there is some functionality that's DetailedDiff here is needed for?

As to ignoreChanges, after some debate in #920 the PF implementation settled on a Pulumi-level-only implementation that would not need to look inside the TF Diffs. That implementation seems to be more closely aligned with the feature docs.

So, the pathway to delete this code is to (1) drop DetailedDiff and (2) switch out ignoreChanges to the different implementation. Given the risks it probably makes more sense to keep maintaining it.

Another phaseout strategy for diff-related issues we discussed with @friel was #1244 - essentially given that we believe upstream resources will be migrating to PF eventually, we want to have an option to try to artificially accelerate this process for individual resources or entire providers as means of fixing diff-related bugs including this one.

Using this new issue to track deciding whether we want to make changes to align these and simplify the implementation for the legacy bridge to match.

@VenelinMartinov
Copy link
Contributor

VenelinMartinov commented May 6, 2024

This sounds like it will not work for GCP: pulumi/pulumi-gcp#1275

GCP has a defaultLabels property on the provider which allows applying labels to all resources. TF takes this into account when diffing and correctly produces a diff but a pulumi-only implementation would miss this.

I don't see a decent way of injecting this information at the pulumi level, so we likely need to maintain our own detailed diff.

I'm interested how PF GCP resources act with default labels given the lack of detailed diff there - I'll open an issue to test our behaviour there. Opened pulumi/pulumi-gcp#1972 to test pf based resources with default labels in GCP.

@VenelinMartinov
Copy link
Contributor

Actually I think #752 already supersedes this - @t0yv0 can we close this issue? Seems like we should rather likely implement DetailedDiff for PF instead.

@t0yv0
Copy link
Member

t0yv0 commented May 6, 2024

Yes the 752 has more detail on a possible way forward.

@VenelinMartinov VenelinMartinov added the resolution/wont-fix This issue won't be fixed label May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/engineering Work that is not visible to an external user resolution/wont-fix This issue won't be fixed
Projects
None yet
Development

No branches or pull requests

3 participants