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

Don't omit an empty detailedDiff #15213

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Don't omit an empty detailedDiff #15213

merged 2 commits into from
Jan 23, 2024

Conversation

komalali
Copy link
Member

@komalali komalali commented Jan 22, 2024

Description

When the detailedDiff object is empty, i.e. {}, it is dropped from the serialized json due to the omitempty. When it is rehydrated on the service, it is rehydrated as nil because it is not present in the json object. That causes this line to be passed through and results in an input diff, which leads to the output we see in the cloud of an import showing a lot of changed properties when it should not (See issue for screenshots).

Fixes pulumi/pulumi-cloud-requests#339

Checklist

  • I have run make tidy to update any new dependencies
  • I have run make lint to verify my code passes the lint check
    • I have formatted my code using gofumpt
  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

@pulumi-bot
Copy link
Contributor

Changelog

[uncommitted] (2024-01-22)

Bug Fixes

  • [sdk/go] Removes omitempty from StepEventMetadata.DetailedDiff
    #15213

Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

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

I really don't like this language, Option[T] could have been a thing.

Interestingly the two other places a DetailedDiff field is included in an object with json tags it's already excluded omitempty, so this at least makes that consistent.

Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for tracking this down, @komalali!

How do we validate the fix and should we add a test?

@Frassle
Copy link
Member

Frassle commented Jan 22, 2024

How do we validate the fix and should we add a test?

We have some places where we just have a test that MarshalJSON gives the text we expect. That would probably cover this sufficiently.

@komalali
Copy link
Member Author

@justinvp I validated the fix by ensuring that we now get an empty map in the resOutputEvent when running PULUMI_DEBUG_COMMANDS=1 pulumi up --event-log file.json when the program is importing a resource via the import resource option. Open to any suggestions on how to translate that to a test.

@komalali komalali added this pull request to the merge queue Jan 23, 2024
Merged via the queue into master with commit 78c7b7b Jan 23, 2024
46 checks passed
@komalali komalali deleted the komal/no-omit-detailed-diff branch January 23, 2024 00:55
@justinvp justinvp mentioned this pull request Jan 25, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 25, 2024
### Features

- [engine] Allow import plugins to define components and logical names.
  [#15199](#15199)

- [cli/display] Incremental improvement on the output when installing
plugins
  [#15201](#15201)

- [sdk] Bake the release version into the SDK
  [#15185](#15185)

- [sdk/go] Parse config paths strictly
  [#15173](#15173)

- [cli/new] Adds pulumi:template tag to `pulumi new` created projects
  [#15056](#15056)

- [auto/nodejs] Add new API to install the Pulumi CLI from the
Automation API
  [#14991](#14991)

- [sdk/python] Add support for Python 3.12


### Bug Fixes

- [engine] Fix a bug where mapping lookup could sometimes lead to an
infinite loop.
  [#15200](#15200)

- [engine] Remove an incorrect assert that resources must have inputs.
  [#15197](#15197)

- [cli/display] Improve output when installing policy packs
  [#15186](#15186)

- [sdk/go] Removes `omitempty` from StepEventMetadata.DetailedDiff
  [#15213](#15213)

- [sdk/go] Replace a deleted type used by Pulumi Cloud code.
  [#15216](#15216)


### Miscellaneous

- [yaml] Upgrade yaml to v1.5.0
  [#15214](#15214)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pulumi Cloud Summary Tab shows drastically different details from CLI during pulumi import
5 participants