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

Revisit patchProposedNewForUnknownBlocks #2247

Closed
t0yv0 opened this issue Jul 25, 2024 · 4 comments
Closed

Revisit patchProposedNewForUnknownBlocks #2247

t0yv0 opened this issue Jul 25, 2024 · 4 comments
Labels
kind/engineering Work that is not visible to an external user resolution/wont-fix This issue won't be fixed

Comments

@t0yv0
Copy link
Member

t0yv0 commented Jul 25, 2024

#2060 introduced a patch (patchProposedNewForUnknownBlocks) to objchange.ProposedNew algorithm vendored from OpenTOFU. This patch solves a usability issue with planning bridged provider update changes, specifically confirmed for sets with unknown elements. There are however some residual concerns whether this is the right fix. We should investigate if we can accomplished the desired goals in a way not involving patching.

Concerns:

  • with the patch more unknowns are surfaced and pushed into provider code as part of PlanResourceChange; if provider authors write code against OpenTOFU/Terraform behavior they may ship code such as plan modifiers that does not expect these unknowns and will panic

  • cross-testing against OpenTOFU becomes more difficult when the behavior deviates intentionally

Alternatives:

  • Possibly the patch can be scoped tighter to sets

  • Perhaps the problem can be solved at the outer Pulumi level, such as ensuring that unknowns from config are preserved at the corresponding positions in the plan, without disturbing the shape of the data passed to provider code

References:

  • See TestUnknowns test suite and the "unknown for set block prop" test case
@pulumi-bot pulumi-bot added the needs-triage Needs attention from the triage team label Jul 25, 2024
t0yv0 added a commit that referenced this issue Jul 25, 2024
With this change it is safe to rerun go generate to vendor objchange.ProposedNew algorithm and its modification is made
explicit by writing out the intentional patch.

Revisiting the patch is tracked in #2247
t0yv0 added a commit that referenced this issue Jul 25, 2024
With this change it is safe to rerun go generate to vendor
objchange.ProposedNew algorithm and its modification is made
explicit by writing out the intentional patch.

Revisiting the patch is tracked in
#2247
@VenelinMartinov
Copy link
Contributor

VenelinMartinov commented Jul 25, 2024

I think this was not limited to sets - a few other test cases also had failures around

In pulumi we can have unknown objects, without the containing collection being unknown - it HCL this is perhaps not expressible (at least I have not found a way to express this) - we were not representing these diffs correctly before the patch and not showing a diff at all there.

@t0yv0
Copy link
Member Author

t0yv0 commented Jul 26, 2024

I'm still concerned it's possible to write a provider that would panic under Pulumi with this but not under TF, but we can keep this around for a bit until we find a real example.

@guineveresaenger guineveresaenger added kind/engineering Work that is not visible to an external user and removed needs-triage Needs attention from the triage team labels Jul 26, 2024
@t0yv0
Copy link
Member Author

t0yv0 commented Sep 27, 2024

Seems like we've rolled this out everywhere and it's ok. Perhaps just an academic possibility. Closing.

@t0yv0 t0yv0 closed this as completed Sep 27, 2024
@pulumi-bot pulumi-bot reopened this Sep 27, 2024
@pulumi-bot
Copy link
Contributor

Cannot close issue:

  • does not have required labels: resolution/

Please fix these problems and try again.

@t0yv0 t0yv0 added the resolution/wont-fix This issue won't be fixed label Sep 27, 2024
@t0yv0 t0yv0 closed this as completed Sep 27, 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

4 participants