pulumi / pulumi-aws

An Amazon Web Services (AWS) Pulumi resource package, providing multi-language access to AWS
Apache License 2.0
465 stars 157 forks source link

Fix redrive_allow_policy JSON diff in upstream provider #4749

Open csssuf opened 1 week ago

csssuf commented 1 week ago

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.

github-actions[bot] commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

csssuf commented 1 week ago

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

flostadler commented 1 week ago

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: https://github.com/pulumi/pulumi-aws/blob/0e03d6a4bcc04b46819b29205b07c6f72a534e5a/provider/provider_nodejs_test.go#L262-L277

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 commented 1 week ago

@csssuf as mentioned in https://github.com/pulumi/pulumi-aws/issues/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 commented 1 week ago

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 commented 1 week ago

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

github-actions[bot] commented 1 week ago

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 commented 1 week ago

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

flostadler commented 1 week ago

/run-acceptance-tests

github-actions[bot] commented 1 week ago

Please view the PR build: https://github.com/pulumi/pulumi-aws/actions/runs/11860700285

github-actions[bot] commented 1 week ago

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 commented 1 week ago

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

github-actions[bot] commented 1 week ago

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 commented 4 days ago

/run-acceptance-tests

github-actions[bot] commented 4 days ago

Please view the PR build: https://github.com/pulumi/pulumi-aws/actions/runs/11890247581

flostadler commented 4 days ago

@csssuf, the regression test seems to still find a diff: https://github.com/pulumi/pulumi-aws/actions/runs/11890247581/job/33131632381#step:17:606

github-actions[bot] commented 4 days ago

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 commented 4 days ago

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

flostadler commented 3 days ago

@csssuf let me know if I can help troubleshoot the issues with the regression test!