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

Fix redrive_allow_policy JSON diff in upstream provider #4749

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

csssuf
Copy link

@csssuf csssuf commented Nov 11, 2024

The sqs.RedriveAllowPolicy resource has the same problem as #2307, where spurious JSON diffs are generated when the actual policies to apply are identical. This PR applies the same fix found in #2529.

Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@flostadler
Copy link
Contributor

Hey @csssuf, thanks for the contribution!

Could you please cut an issue describing what's currently not working and whether there's any workarounds?

Given that this introduces a patch for the upstream terraform provider we should also cut an issue on their end describing the problem. I'm happy to do that, just need an issue on our end first so I have enough context.

@csssuf
Copy link
Author

csssuf commented Nov 12, 2024

No problem - I'll put together a minimal repro to include.

@csssuf
Copy link
Author

csssuf commented Nov 12, 2024

Opened #4760 - let me know if it needs anything else!

@flostadler
Copy link
Contributor

Thanks @csssuf! I'll take a look if this repros in upstream Terraform and cut the necessary issues if it does.

We're generally trying to be careful about introducing patches because they're an increased maintenance burden. I'll bring this up for discussion internally once I figured out if that's an upstream issue or needs fixing on the Pulumi side.

Ideally we also need a regression test for this. A good example is this one here that also tests for spurious diffs:

func TestRegress4446(t *testing.T) {
skipIfShort(t)
dir := filepath.Join("test-programs", "regress-4446")
cwd, err := os.Getwd()
require.NoError(t, err)
providerName := "aws"
options := []opttest.Option{
opttest.LocalProviderPath(providerName, filepath.Join(cwd, "..", "bin")),
opttest.YarnLink("@pulumi/aws"),
}
test := pulumitest.NewPulumiTest(t, dir, options...)
upResult := test.Up(t)
t.Logf("#%v", upResult.Summary)
result := test.Preview(t, optpreview.ExpectNoChanges())
t.Logf("#%v", result.ChangeSummary)
}

If you don't have the necessary setup, I can also add the regression test to the PR based on the repro you added to the issue. Let me know if you'd like some support here.

@flostadler
Copy link
Contributor

@csssuf as mentioned in #4760 I was able to reproduce this in the upstream Terraform provider.

We've decided to take a patch for this, at the same time I'll work on getting this fixed upstream.

Let me know if you'd like some help with the regression tests for getting this over the finish line!

@csssuf
Copy link
Author

csssuf commented Nov 13, 2024

Thanks for filing upstream! I should be able to get a regression test added here in a bit, looks relatively straightforward. Is there any rhyme or reason to which language to implement the test in for language-agnostic bugs like this?

@flostadler
Copy link
Contributor

Thanks for filing upstream! I should be able to get a regression test added here in a bit, looks relatively straightforward. Is there any rhyme or reason to which language to implement the test in for language-agnostic bugs like this?

Awesome! You can choose the language for the regression test. I guess python would be the easiest given that the repro is using that. Generally we try to name the regression tests after the issue number (i.e. regress-4760 in this case).

This is an example of a python regression test you can copy the boilerplate from: https://github.com/pulumi/pulumi-aws/tree/0e03d6a4bcc04b46819b29205b07c6f72a534e5a/provider/test-programs/regress-4457

Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@csssuf
Copy link
Author

csssuf commented Nov 14, 2024

@flostadler added what seems like the correct test to me - let me know if it looks incorrect!

@flostadler
Copy link
Contributor

/run-acceptance-tests

Copy link

Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@csssuf
Copy link
Author

csssuf commented Nov 15, 2024

Oops, had to fix up the test program structure a bit - pushed a fix.

Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@flostadler
Copy link
Contributor

/run-acceptance-tests

Copy link

@flostadler
Copy link
Contributor

Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@flostadler
Copy link
Contributor

@csssuf I also merged the latest master branch into your PR to fix some unrelated tests

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.

2 participants