-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws: API Gateway resources #4295
provider/aws: API Gateway resources #4295
Conversation
@radeksimko I still need to add acceptance tests for both |
There are still a some things left to do:
but the overall structure has settled now. |
What's the status of this? |
@hlandau it works, I need to add some more tests and refactor it a little. Would love a review though, b/c it's my first terraform PR with lots of changes. |
@nicolai86 I have done some additional work with this PR to support integration responses, which changes the syntax a little. Also it's quite difficult to create a dependency from gateway to deployment that works right now as depends_on doesn't seem to interpolate.
|
Is the complexity of properly supporting all the PATCH requests needed to update a method in place worth it? Any api method is likely to be near the leaves of the dependency tree |
@RyanRoberts you probably based your work off of an earlier version of the code - I've split thinks up into multiple resources since then. Take a look at the PR top, there's the current syntax noted. This also reduced most of the issues with PATCHing. For an initial version I'd even remove this entirely and just recreate everythin when attributes change, just to keep the diff smaller |
Ah, yes. This looks much better. |
This is exactly the PR I've been looking for! Hopefully it can get merged and out in to a release soon, as I'd very much like to use these new resources to configure API Gateway for a stack I'm helping to manage. |
How would I integrate this with a Lambda function? |
@brycefisher You need to use the AWS integration type, construct the lambda invocation URI (arn:aws:apigateway:${var.region}:lambda:path/2015-03-31/functions/${functionname}/invocations ) and post to it, unsure if it's provided by the lambda resource itself. You will also likely end up using request templates to pass configuration / context
|
@brycefisher @ryansroberts is correct, you'd need to construct an url for that. However, instead of |
I should possibly put a PR in to export a computed invocation uri from the
|
There are upcoming changes to the API Gateway ..API that make both of our approaches pretty pointless. We should have the ability to create a much cleaner and simpler resource for this next month |
@ryansroberts do you have a commit to check out? |
@ryansroberts what are the upcoming changes? |
Even if there are potentially changes in the pipeline, a working api gateway resource today and a refactoring (maybe) in a month is far more pragmatic than waiting for "should." Is there any reason to not pull these resources? |
My branch works for my use case, but It's going to be quite a large rework. Depends on your use case / how soon you need, this is currently quite hard to manage due to implicit dependencies. A closer intermediate solution would probably be to use the AWS labs swagger importer with https://www.terraform.io/docs/providers/template/r/file.html |
👍 for the aws swagger importer + the terraform template. The web console can export a swagger definition to help you understand how it works (the documentation on how to use swagger with aws is a bit lacking IMHO). Not being a Java / maven expert, I simply used the Dockerfile provided by AWS Labs and it worked like a charm. I agree with @fuwjax about the value of having api gateway support directly in terraform sooner rather than later. I quickly scanned through the files on this PR and I didn't see the implicit dependencies, only the aws-go-sdk. What are the implicit dependencies you speak of? |
By implicit dependencies I meant between the method / resource / deployment resources. Currently you need to use interpolation to ensure things are constructed in the right order. |
@@ -107,6 +107,15 @@ func Provider() terraform.ResourceProvider { | |||
"aws_ami": resourceAwsAmi(), | |||
"aws_ami_copy": resourceAwsAmiCopy(), | |||
"aws_ami_from_instance": resourceAwsAmiFromInstance(), | |||
"aws_api_gateway": resourceAwsApiGateway(), |
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.
Is there any reason why can't we call this resource aws_api_gateway_rest_api
and make it match the AWS API naming convention?
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.
I found aws_api_gateway_rest_api
quite long. But I've changed it anyhow.
<a href="/docs/providers/aws/r/api_gateway_rest_api.html">aws_api_gateway_rest_api</a> | ||
</li> | ||
<li<%= sidebar_current("docs-aws-resource-api-gateway-deployment") %>> | ||
<a href="/docs/providers/aws/r/api_gateway_deployment.html">api_gateway_deployment</a> |
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.
The link title here should be aws_api_gateway_deployment
(including the aws_
).
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.
corrected!
I think it's very close to land in We also usually keep all validation functions in One last thing, that would be very helpful before I merge this - I may be asking for too much (depending on how skilled you're in git 😃 ) - but would you mind squashing those https://help.github.com/articles/about-git-rebase/ If you don't feel confident enough about rebasing, I'll be happy to take care of that - just let me know. |
@radeksimko I've moved the validation function, added a very tiny test & clean up the commits according to your suggestion. |
I just rescanned it thoroughly again, reran all tests, and it looks ok. There is a few nitpicks, but we can deal with those in a separate PR. 🚢 |
provider/aws: API Gateway resources
@radeksimko thanks a lot for your review! Great help for my very first terraform PR! |
Whoa!!! So awesome. |
Well done guys! |
Now that #2143 is fixed it's time to revisit the parameter handling and probably also remove the |
@nicolai86 Agreed. PRs welcomed. |
* provider/aws: Re-implement api gateway parameter handling this PR cleans up some left overs from PR #4295, namely the parameter handling. now that GH-2143 is finally closed this PR does away with the ugly `request_parameters_in_json` and `response_parameters_in_json` hack. * Add deprecation message and conflictsWith settings following @radeksimko s advice, keeping the old code around with a deprecation warning. this should be cleaned up in a few releases * provider/aws: fix missing append operation * provider/aws: mark old parameters clearly as deprecated * provider/aws work around #8104 following @radeksimko s lead * provider/aws fix cnp error
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Hello,
I implemented many new resources for the AWS API Gateway.
The new resources are:
aws_api_gateway_rest_api
- manage REST APIsaws_api_gateway_resource
- manage API resourcesaws_api_gateway_method
- manage resource methodsaws_api_gateway_method_response
- manage resource method responsesaws_api_gateway_integration
- manage resource method integrationaws_api_gateway_integration_response
- manage resource integration responsesaws_api_gateway_model
- manage resource modelsaws_api_gateway_deployment
- manage REST API deploymentsaws_api_gateway_api_key
- manage REST API keysExample usage as of now:
Feedback and critic are very welcome.
Acceptance Test Status: