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

provider/aws: API Gateway additional resources #6175

Closed
wants to merge 5 commits into from

Conversation

ryansroberts
Copy link
Contributor

The new swagger import feature, implemented as a separate resource
Domain name and Base path mappings.

Domain name update currently disabled, as it's not actually implemented in AWS yet, a couple of weeks apparently.

@ryansroberts
Copy link
Contributor Author

Should pass with the latest aws sdk, importrestapi recently added

@radeksimko radeksimko changed the title Apigateway additional resources provider/aws: API Gateway additional resources Apr 16, 2016
@catsby
Copy link
Contributor

catsby commented Apr 19, 2016

Hey @ryansroberts could you bump the SDK that's vendored? We've got the process outlined here, it's a 3 or 4 step easy process:

That should make TravisCI happy.

Just curious, there are keys in this PR, I hope those are fake?

@catsby catsby added the waiting-response An issue/pull request is waiting for a response from the community label Apr 19, 2016
@ryansroberts
Copy link
Contributor Author

Yes, they are self signed fake keys :)

@ryansroberts
Copy link
Contributor Author

ah, ok. Sorry, my golang is pretty basic. Will do

@francknouama
Copy link

👍

@radeksimko radeksimko self-assigned this May 3, 2016
@josh-padnick
Copy link

josh-padnick commented Jun 22, 2016

Any update on this PR? I'd love to utilize this in my project since we need CORS settings for API Gateway and the only way to do this via Terraform, as far as I can tell right now, would be for Terraform to support the API Gateway Import API.

@ryansroberts
Copy link
Contributor Author

@josh-padnick You can do it with the existing resources, but its a huge faff. I'd kind of like to get this (among others) merged too. Upstream awslib should be good now

@josh-padnick
Copy link

@ryansroberts Thanks for the head's up! I realized I was reading the docs too literally and that enabling CORS really just amounts to setting the right headers. What is your primary use case, then, for the Swagger import? Is it just that you find it easier to declare an API structure using a Swagger JSON/YAML versus a combination of Terraform resources?

@binarymatt
Copy link

@ryansroberts i've been testing this pr internally and we found one issue. In particuar, if you change the swagger definition, this code recreates the api gateway. Unfortunately, this means a new url endpoint is created. Locally, i've modified this pr to update a current definition if the swagger is updated. Would you be willing to take a look and possible bring it into this pr?

@ryansroberts
Copy link
Contributor Author

Yes, not a problem. I'll have a look this weekend, sorry about that

@ryansroberts
Copy link
Contributor Author

@josh-padnick I had a forked version of the original gateway resource stuff, it gets quite fiddly as there's a cyclic dependency, plus the definitions are bulky. It's much easier on the eyes and fingers to run a swagger definition through terraform template syntax than have 80 odd terraform resources for a 10 method api.

@apparentlymart apparentlymart removed the waiting-response An issue/pull request is waiting for a response from the community label Jul 15, 2016
@radeksimko radeksimko removed their assignment Aug 7, 2016
@apparentlymart
Copy link
Contributor

Hi, @ryansroberts! Sorry for the long silence here.

I extracted the base path mapping part of your work here and combined it with @jarrodjackson's work on domain names to produce what was merged in #8353, which addresses the first to parts of your PR here. (I didn't notice your work overlapped with Jarrod's until I'd already rebased and reviewed his, which was why I ended up blending to two...)

Unfortunately that means that your patch here now has conflicts with what's in master.

The Swagger part of your change here still looks good, and it'd be cool if we could split it off from the rest and produce a smaller patch that only includes that. I'm happy to help with this if you'd like me to, since I know my partial-merging of your patch here has added some complexity.

@radeksimko
Copy link
Member

I was looking at the Swagger import functionality today and I noticed that the import is one-way - i.e. if we were to treat it as a resource which may be updated then any update would cause outage and require re-deployment.

I appreciate the time @ryansroberts invested into this and I still think it would be great if Terraform supported this, but I think the best way to support this might be an addition to aws_api_gateway_rest_api, specifically the import functionality there. We do treat terraform import as one way already - so there would be no surprises.

Admittedly that ^ would require some changes in the core import logic as I think we now only support ID-based imports.

In ideal case you'd be able to do something like

$ terraform import aws_api_gateway_rest_api.foo -from-swagger="$(cat definition.json)"

And then we'd have to lookup all resources that have been created as an effect of that import -
methods, integrations, method responses, integration responses etc.

@apparentlymart
Copy link
Contributor

I have some code at work that takes a swagger definition with the API Gateway extensions and produces a JSON config file for Terraform to replicate that config as distinct resources. I think that gets to what you are looking for here, right @radeksimko? Perhaps I could look at open sourcing that after separating it better from the app it is supporting, and that would get us a Swagger mechanism without changing Terraform's import design.

(I think generating config is superior to import for "new" (not already created) resources since it generates the config rather than just the state, and so the user doesn't have to do lots of work to match the config to what was imported.)

@radeksimko
Copy link
Member

@apparentlymart Sure, that's also a solution. It would be probably a lot of work to create a matching config after such import, but at some point the native Terraform import functionality will generate the config for you - just a matter of time ;-)

@sthulb
Copy link

sthulb commented Nov 4, 2016

@ryansroberts / @catsby Is this functionality something that would be accepted? If it's not being actively worked on, I'd be happy to work on adding a swagger resource to TF.

@justinabrahms
Copy link

I have a preference for not generating terraform config based on the swagger file. Terraform may not always support 100% of what can be described in swagger from importing.

@apparentlymart
Copy link
Contributor

Hello @ryansroberts, and thanks for working on this!

As part of the the Terraform 0.10 release earlier this year, all of the Terraform providers were moved to their own repositories in the terraform-providers GitHub organization, and removed from the Terraform Core repository.

Unfortunately due to the fact that new issues and pull requests are being opened constantly, it was not possible for the various provider maintainers to merge all outstanding pull requests before this split, and there is no automatic way to migrate a pull request to a new repository.

As a result, this pull request can sadly no longer be applied as-is. However, it seems that a similar change was made in hashicorp/terraform-provider-aws#1197 that allows Swagger import to API gateway, so I'm going to close this one.

Thanks again for working on this, and sorry it was not able to be merged before the provider repository changes.

@ghost
Copy link

ghost commented Apr 6, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants