Support API Gateway #1108
Merged
Support API Gateway #1108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…romAPIName funciton to operator
deliahu
reviewed
Jun 3, 2020
There was a problem hiding this comment.
This looks really good, nice job figuring out the AWS APIs / CLI!
I added some comments, and I also made some edits. Mostly they were minor edits or rearranging of code. Here are some of the things I changed:
- I replaced occurrences of e.g.
loadBalancerScheme.String() == "internet-facing"withloadBalancerScheme == clusterconfig.InternetFacingLoadBalancerScheme - In
create_gateway_integration.py, I passedclient_elbinto the helper functions - You can ignore the diff in
install.sh, since that was just minor formatting - I moved the code around in
lib/aws/apigateway.go, it will probably be easier to look at the new file instead of looking at the diff. The main change is that I tried to make everything in that file not cortex-specific, since we try to keep cortex-specific code out of thelibpackages. There were some other more minor changes in this file. - I updated
operator/gateway.goto use the updated functions fromlib/aws/apigateway.go. This file will also be easier to see if you open the new version rather than comparing the diff. - In
operator/api.go, I rearranged it so that it will still attempt to delete all resources even if API gateway deletion fails. I also updatedGetSpecForRunningAPI()andgetAPIIDForRunningAPI()to return nil/empty if the API is not running (rather than panic)
I didn't try running all of my changes, since I wanted to run them by you first; what do you think, does this all make sense?
Also, I noticed that you decided to query the API Gateway from AWS when necessary, rather than adding the API Gateway information to the config package. Is there a reason you went with this approach instead of loading it in config.Init()?
deliahu
reviewed
Jun 11, 2020
vishalbollu
approved these changes
Jun 12, 2020
| If you've setup API gateway and want to delete it, please follow these [instructions](../guides/api-gateway.md#cleanup). | ||
|
|
||
| If you've configured HTTPS by specifying an SSL Certificate for a subdomain in your cluster configuration, you may wish to remove the SSL Certificate and Hosted Zone for the domain by following these [instructions](../guides/subdomain-https-setup.md#cleanup). | ||
| If you've configured a custom domain for your APIs, you may wish to remove the SSL Certificate and Hosted Zone for the domain by following these [instructions](../guides/custom-domain.md#cleanup). |
There was a problem hiding this comment.
nit: you may wish to remove -> you may remove
deliahu
approved these changes
Jun 15, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #1077
checklist:
make testandmake lintsummary.md(view in gitbook after merging)