-
Notifications
You must be signed in to change notification settings - Fork 104
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
Correctly support apiKeySource in classic provider #1159
Conversation
We were previously passing apiKeySource only via Swagger. Even though AWS API Gateway does include the API key source in the swagger on output, it apparently does not read it from the swagger on updates. However, it does support passing it outside of the Swagger, so we just pass this along directly to the RestAPI constructor is specified. We could in principle *stop* sending it in the Swagger as well (since I don't think it's read from there at all), but I am avoiding changing that in case it has other subtle impacts on behaviour.
7a60bdb
to
6df3adc
Compare
Thank you @lukehoban! Your PR description doesn't say, so I gotta ask, did you test this change manually? It would be awesome to have a two-step test in |
Agreed - I'll get a test added before we merge this. |
@thomas11 Test added - ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Luke! I've filed an issue for us to port this fix into pulumi-aws-apigateway as well
Great. I’ve got a PR nearly ready for that - so happy to finish that up once this is merged. |
Tracked in pulumi/pulumi-aws-apigateway#112. |
Ports the fix from pulumi/pulumi-awsx#1159 into this package. Requires an awkward workaround for #111 (the workaround also takes a dependency on the currently broken behaviour, so this test will start failing when #111 is fixed). Fixes #110.
We were previously passing apiKeySource only via Swagger. Even though AWS API Gateway does include the API key source in the swagger on output, it apparently does not read it from the swagger on updates.
However, it does support passing it outside of the Swagger, so we just pass this along directly to the RestAPI constructor is specified.
We could in principle stop sending it in the Swagger as well (since I don't think it's read from there at all), but I am avoiding changing that in case it has other subtle impacts on behaviour.
Fixes #1057.