search

pulumi / pulumi-aws-apigateway

Apache License 2.0
12 stars 5 forks source link

Remove automatic setting of binary media types header #119

Closed pierskarsenbarg closed 9 months ago

pierskarsenbarg commented 9 months ago

The binaryMediaTypes property of the 'Api' resource of the RestAPI component would get overriden because of a default applied to an amazon header: "x-amazon-apigateway-binary-media-types": ["*/*"].

This PR gets rid of it and adds a regression test, as well as upgrade tests in order to make sure we don't replace any resources in the process.

See comment here: https://github.com/pulumi/pulumi-aws-apigateway/issues/118#issuecomment-1850035758

Fixes: #118

danielrbradley commented 9 months ago

Is this not going to break anyone who's currently expecting the binary media types to be set automatically?

pierskarsenbarg commented 9 months ago

No because we're setting it here by default: https://github.com/pulumi/pulumi-aws-apigateway/blob/main/provider/cmd/pulumi-resource-aws-apigateway/apigateway/api.ts#L661 But this one <- was being overwritten by the swagger body if you updated the routes array.

VenelinMartinov commented 9 months ago

This is probably ready for merging. It'd be great if we can have another quick review from someone in @pulumi/providers!

We've changed the behaviour so that the change no longer causes any replacements and added a regression test as well as upgrade tests.

thomas11 commented 9 months ago

This is probably ready for merging. It'd be great if we can have another quick review from someone in @pulumi/providers!

The new test fails without the fix?

VenelinMartinov commented 9 months ago

Yup, I verified it here: https://github.com/pulumi/pulumi-aws-apigateway/actions/runs/7247196392/job/19740649953 and then restored the fix in the next commit.

pierskarsenbarg commented 9 months ago

For some reason my test that wasn't any good was still around. I suspect because of merging. I'll re-run the tests if I need to but the test program wasn't there so I think it would have failed anyway

VenelinMartinov commented 9 months ago

Note that users who previously specified binaryMediaTypes would get replacements after upgrading to this change.

This is probably fine since the resource wouldn't have worked correctly for them before and the resources are stateless.