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

Detailed diff shows diff on __defaults when number of elements changes #2400

Closed
VenelinMartinov opened this issue Sep 6, 2024 · 5 comments · Fixed by #2405
Closed

Detailed diff shows diff on __defaults when number of elements changes #2400

VenelinMartinov opened this issue Sep 6, 2024 · 5 comments · Fixed by #2405
Assignees
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed

Comments

@VenelinMartinov
Copy link
Contributor

What happened?

When adding/removing elements from a block type we seem to display a diff on the __defaults:

  ~ prov:index/test:Test: (update)
      [id=newid]
      [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes]
    ~ listBlocks: [
        ~ [0]: {
                + __defaults: []
                  prop      : "val1"
              }
        ~ [1]: {
                + __defaults: []
                  prop      : "val2"
              }
        + [2]: {
                + prop      : "val3"
              }
      ]

Example

#2398
list block element added back

Output of pulumi about

.

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@VenelinMartinov VenelinMartinov added the kind/bug Some behavior is incorrect or out of spec label Sep 6, 2024
@pulumi-bot pulumi-bot added the needs-triage Needs attention from the triage team label Sep 6, 2024
@VenelinMartinov VenelinMartinov removed the needs-triage Needs attention from the triage team label Sep 6, 2024
VenelinMartinov added a commit that referenced this issue Sep 16, 2024
This adds tests for the detailed diff output of
- List blocks
- Set blocks
- MaxItemsOne list blocks

for adding, removing, changing the property and adding, removing,
changing nested properties in the block.

A few issues here:
- #2234 affects
quite a few of the cases
- #2399 diffs
are displayed on nested properties which are not specified when a block
is removed
- #2400 diffs
are displayed on the `__defaults` property when the number of elements
in the block changes.
@VenelinMartinov
Copy link
Contributor Author

This one is not an issue with the detailed diff calculation:

detailedDiff=map[string]*pulumirpc.PropertyDiff{
           "maxItemsOneBlock": &pulumirpc.PropertyDiff{
               state:         impl.MessageState{},
               sizeCache:     0,
               unknownFields: nil,
               Kind:          4,
               InputDiff:     false,
           },
           "maxItemsOneBlock.prop": &pulumirpc.PropertyDiff{
               state:         impl.MessageState{},
               sizeCache:     0,
               unknownFields: nil,
               Kind:          4,
               InputDiff:     false,
           },
       }

There is no __defaults marked here - instead this seems to be an issue with the news - the block object contains the __defaults key when it looks like there is no reason for that:

// Each object-typed value contains metadata about the properties that were set using default values under a

Non-top level defaults don't seem to get used anywhere.

Also:

// TODO: We should probably remove the extraneous defaultsKey here.
suggests the __defaults are not necessary.

@VenelinMartinov
Copy link
Contributor Author

Perhaps we can remove all empty nested defaults arrays.

@VenelinMartinov
Copy link
Contributor Author

this is all sidestepped if we only return the bottom-most property which has a diff, instead of all the properties in the tree - this works so much better.

@t0yv0
Copy link
Member

t0yv0 commented Oct 1, 2024

__defaults is a special property used by the bridge and it should never be presented in the diffs AFAIK. It's "about" some other properties. Whether they were populated from a default value or not.

@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/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
3 participants