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

Never show property diffs for OpSame #16024

Merged
merged 3 commits into from May 2, 2024
Merged

Never show property diffs for OpSame #16024

merged 3 commits into from May 2, 2024

Conversation

Frassle
Copy link
Member

@Frassle Frassle commented Apr 22, 2024

Description

Fixes #15944.

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

pulumi-bot commented Apr 22, 2024

Changelog

[uncommitted] (2024-05-02)

Bug Fixes

  • [cli/display] Avoid showing a diff when the provider returns DIFF_NONE
    #16024

@tgummerer
Copy link
Collaborator

Here's the test output of the display tests with this change applied: https://gist.github.com/tgummerer/a8e3cc9f097115dc3b53e4dd8c17beb9

Aside from the moved lines which I'm still working on, I think the diff makes sense. E.g.

    test_plan.go:380: 
        	Error Trace:	/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/test_plan.go:380
        	            				/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/test_plan.go:238
        	            				/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/test_plan.go:159
        	            				/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/test_plan.go:143
        	            				/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/import_test.go:1215
        	Error:      	Not equal: 
        	            	expected: "<{%reset%}>  pulumi:providers:pkgA: (same)\n<{%reset%}>    [id=5cd80e19-0f6f-490d-885f-9a3ff83c80a5]\n<{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pulumi:providers:pkgA::default]\n<{%reset%}><{%reset%}><{%reset%}>  pkgA:m:typA: (same)\n<{%reset%}>    [id=actual-id]\n<{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pkgA:m:typA::resA]\n<{%reset%}><{%fg 3%}>  ~ foo: <{%reset%}><{%fg 3%}>\"<{%reset%}><{%reset%}>bar<{%reset%}><{%fg 3%}>\"<{%reset%}><{%fg 3%}> => <{%reset%}><{%fg 3%}>\"<{%reset%}><{%reset%}>bar<{%reset%}><{%fg 2%}>z<{%reset%}><{%fg 3%}>\"\n<{%reset%}><{%reset%}><{%reset%}>    --outputs:--<{%reset%}>\n<{%reset%}>    foo : <{%reset%}><{%reset%}>\"bar\"<{%reset%}><{%reset%}>\n<{%reset%}><{%reset%}>    frob: <{%reset%}><{%reset%}>1<{%reset%}><{%reset%}>\n<{%reset%}><{%fg 1%}>- pulumi:pulumi:Stack: (delete)\n<{%fg 1%}>    [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test]\n<{%reset%}><{%reset%}><{%fg 13%}><{%bold%}>Resources:<{%reset%}>\n    <{%fg 1%}>- 1 deleted<{%reset%}>\n    1 unchanged\n\n<{%fg 13%}><{%bold%}>Duration:<{%reset%}> 0s\n"
        	            	actual  : "<{%reset%}>  pulumi:providers:pkgA: (same)\n<{%reset%}>    [id=5cd80e19-0f6f-490d-885f-9a3ff83c80a5]\n<{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pulumi:providers:pkgA::default]\n<{%reset%}><{%reset%}><{%reset%}>  pkgA:m:typA: (same)\n<{%reset%}>    [id=actual-id]\n<{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pkgA:m:typA::resA]\n<{%reset%}><{%reset%}><{%reset%}>    --outputs:--<{%reset%}>\n<{%reset%}>    foo : <{%reset%}><{%reset%}>\"bar\"<{%reset%}><{%reset%}>\n<{%reset%}><{%reset%}>    frob: <{%reset%}><{%reset%}>1<{%reset%}><{%reset%}>\n<{%reset%}><{%fg 1%}>- pulumi:pulumi:Stack: (delete)\n<{%fg 1%}>    [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test]\n<{%reset%}><{%reset%}><{%fg 13%}><{%bold%}>Resources:<{%reset%}>\n    <{%fg 1%}>- 1 deleted<{%reset%}>\n    1 unchanged\n\n<{%fg 13%}><{%bold%}>Duration:<{%reset%}> 0s\n"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -6,3 +6,2 @@
        	            	 <{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pkgA:m:typA::resA]
        	            	-<{%reset%}><{%fg 3%}>  ~ foo: <{%reset%}><{%fg 3%}>"<{%reset%}><{%reset%}>bar<{%reset%}><{%fg 3%}>"<{%reset%}><{%fg 3%}> => <{%reset%}><{%fg 3%}>"<{%reset%}><{%reset%}>bar<{%reset%}><{%fg 2%}>z<{%reset%}><{%fg 3%}>"
        	            	 <{%reset%}><{%reset%}><{%reset%}>    --outputs:--<{%reset%}>
        	Test:       	TestImportInputDiff

Comes from a test where the comment explicitly says "Check that if a provider normalizes an input we don't display that as a diff.". We did display the diff before, but don't do now.

One thing I'm not sure about is diffs like this:

--- FAIL: TestStackReferenceRegister (0.07s)
    test_plan.go:380: 
        	Error Trace:	/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/test_plan.go:380
        	            				/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/test_plan.go:238
        	            				/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/test_plan.go:159
        	            				/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/test_plan.go:143
        	            				/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/test_plan.go:581
        	            				/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/test_plan.go:606
        	            				/home/tgummerer/work/pulumi/main/pkg/engine/lifecycletest/pulumi_test.go:1184
        	Error:      	Not equal: 
        	            	expected: "<{%reset%}>  pulumi:providers:pulumi: (same)\n<{%reset%}>    [id=66b11ec3-40c1-49e7-a4ad-a75713cda9b3]\n<{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pulumi:providers:pulumi::default]\n<{%reset%}><{%reset%}><{%reset%}>  pulumi:pulumi:StackReference: (same)\n<{%reset%}>    [id=c9e3bcf1-a501-437f-9f3f-abdcb2c06d88]\n<{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pulumi:pulumi:StackReference::other]\n<{%reset%}><{%reset%}>    name: <{%reset%}><{%reset%}>\"other\"<{%reset%}><{%reset%}>\n<{%reset%}><{%reset%}><{%reset%}>    --outputs:--<{%reset%}>\n<{%reset%}>    outputs          : <{%reset%}><{%reset%}>{\n<{%reset%}><{%reset%}>        foo: <{%reset%}><{%reset%}>\"bar\"<{%reset%}><{%reset%}>\n<{%reset%}><{%reset%}>    }<{%reset%}><{%reset%}>\n<{%reset%}><{%fg 13%}><{%bold%}>Resources:<{%reset%}>\n    1 unchanged\n\n<{%fg 13%}><{%bold%}>Duration:<{%reset%}> 0s\n"
        	            	actual  : "<{%reset%}>  pulumi:providers:pulumi: (same)\n<{%reset%}>    [id=66b11ec3-40c1-49e7-a4ad-a75713cda9b3]\n<{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pulumi:providers:pulumi::default]\n<{%reset%}><{%reset%}><{%reset%}>  pulumi:pulumi:StackReference: (same)\n<{%reset%}>    [id=c9e3bcf1-a501-437f-9f3f-abdcb2c06d88]\n<{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pulumi:pulumi:StackReference::other]\n<{%reset%}><{%reset%}><{%reset%}>    --outputs:--<{%reset%}>\n<{%reset%}>    outputs          : <{%reset%}><{%reset%}>{\n<{%reset%}><{%reset%}>        foo: <{%reset%}><{%reset%}>\"bar\"<{%reset%}><{%reset%}>\n<{%reset%}><{%reset%}>    }<{%reset%}><{%reset%}>\n<{%reset%}><{%fg 13%}><{%bold%}>Resources:<{%reset%}>\n    1 unchanged\n\n<{%fg 13%}><{%bold%}>Duration:<{%reset%}> 0s\n"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -6,3 +6,2 @@
        	            	 <{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pulumi:pulumi:StackReference::other]
        	            	-<{%reset%}><{%reset%}>    name: <{%reset%}><{%reset%}>"other"<{%reset%}><{%reset%}>
        	            	 <{%reset%}><{%reset%}><{%reset%}>    --outputs:--<{%reset%}>
        	Test:       	TestStackReferenceRegister

Which in raw data looks like

diff --git a/pkg/engine/lifecycletest/testdata/output/TestStackReferenceRegister/-2-0/diff.stdout.txt b/pkg/engine/lifecycletest/testdata/output/TestStackReferenceRegister/-2-0/diff.stdout.txt
index fea9a8f72b..b4d724526b 100644
--- a/pkg/engine/lifecycletest/testdata/output/TestStackReferenceRegister/-2-0/diff.stdout.txt
+++ b/pkg/engine/lifecycletest/testdata/output/TestStackReferenceRegister/-2-0/diff.stdout.txt
@@ -1,10 +1,9 @@
 <{%reset%}>  pulumi:providers:pulumi: (same)
-<{%reset%}>    [id=f52fcf33-434b-4e82-b0cd-da5af0b43927]
+<{%reset%}>    [id=c559bc28-ebd5-4c12-8d22-a4a954ddec2f]
 <{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pulumi:providers:pulumi::default]
 <{%reset%}><{%reset%}><{%reset%}>  pulumi:pulumi:StackReference: (same)
-<{%reset%}>    [id=a11a5c67-627d-4f7e-b78d-779a87b2905e]
+<{%reset%}>    [id=a8c241ad-9ad9-484d-a025-b2da057aeb13]
 <{%reset%}><{%reset%}>    [urn=urn:pulumi:test::test::pulumi:pulumi:StackReference::other]
-<{%reset%}><{%reset%}>    name: <{%reset%}><{%reset%}>"other"<{%reset%}><{%reset%}>
 <{%reset%}><{%reset%}><{%reset%}>    --outputs:--<{%reset%}>
 <{%reset%}>    outputs          : <{%reset%}><{%reset%}>{
 <{%reset%}><{%reset%}>        foo: <{%reset%}><{%reset%}>"bar"<{%reset%}><{%reset%}>

So it removes some output we were showing before for providers, but that doesn't look like a diff 🤔

@Frassle
Copy link
Member Author

Frassle commented Apr 29, 2024

Yeh that looks fine to me

I looked through the diff of the testdata here and it all looks
reasonable.  We are expecting a bunch less output because we don't
actually want to show it to users.
@tgummerer tgummerer marked this pull request as ready for review April 30, 2024 12:00
@tgummerer tgummerer requested a review from a team as a code owner April 30, 2024 12:00
@Frassle Frassle added this pull request to the merge queue May 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 2, 2024
@tgummerer tgummerer added this pull request to the merge queue May 2, 2024
Merged via the queue into master with commit dc12644 May 2, 2024
49 checks passed
@tgummerer tgummerer deleted the fraser/opSame branch May 2, 2024 14:07
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.

Diff displayed even when provider returns DIFF_NONE and empty detailed diff
3 participants