-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(apigateway): create RestApi from an OpenAPI spec #7372
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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 for submitting this PR.
Before I get into the code, I believe some of the properties in RestApiProps
may conflict or not be compatible when an openapi definition is provided, such as, binaryMediaTypes
and cloneFrom
. What happens when they are specified along with the openapi definition?
Would it be cleaner if there's a different entry point when starting off with a OpenAPI definition?
Taking a look at the CloudFormation Docs for the Rest API, there appears to be no info on what happens when these additional props are given. Not wanting to assume everything's OK, I tried a bunch of different configurations in the integration tests - passing in different combinations of Some more information on Because users can specify any of the other Rest API props, I don't think moving this to a static constructor/different entry point for an API definition (i.e. |
Thanks for trying these out! While there were no errors, what was the final behaviour? There are two ways to go about this - either document this behaviour or create a different entry point (either a
Can you expand? Wouldn't this be more confusing, especially when there are two different values across these two definitions? Further, were you able to check if calling You'll need to expand on the README and add details on this behaviour. |
Thanks for the questions, I'll see if I can clarify: The OpenAPI specification always takes precedent over CloudFormation constructs. For example, if I create a GET method on a /test resource in the specification, then call I can confirm calling I agree that just "telling" the user this precedence in the README isn't enough - this could lead to "shooting yourself in the foot", which I imagine is not what CDK wants to allow the user to do. I will resubmit the PR with the README edited and a clear separation in code between creating a Rest API with an OpenAPI specification and the current method ( |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Updated the PR with the separation. This was a bigger change than I thought - adding an OpenAPI specification makes some props irrelevant, but not all (like the binaryMediaTypes you mentioned but not the CloudWatch roles/policies and domain names). A static |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thanks a lot @bdgmb2 for this! |
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.
This is looking much better. Thanks for making these changes.
I've have not gone through through this completely, but publishing my comments so far.
Hello everyone, my apologies as it may appear this PR hasn't gotten any attention from me over the past couple days. |
@bdgmb2 - Thanks for letting us know that this is going to be on your spare time. We really appreciate the effort! 😊 The PR #7097 has no decisions that will impact this PR, so you can proceed with the changes suggested. I had only referenced to your PR - this one - as how the OpenAPI support should be modeled. I would appreciate it if you could comment on here, at any point, if you're unable to take this PR forward. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Commit Message
feat(apigateway): create RestApi from an OpenAPI spec
Co-authored-by: Niranjan Jayakar [email protected]
Ability to import an OpenAPI definition to an API Gateway Rest API.
The definition can either be a file as a local asset, inline JSON or a
key in an S3 bucket.
closes #4421
End Commit Message
Changes in this PR were modeled after the aws-lambda package, specifically how that package deals with handling lambda function code. This PR introduces no breaking changes: adding an API definition is optional, and developers can continue to add resources and methods to an API even after an API definition is specified if they choose.
This does not close issue #1461 - that issue is asking for a much bigger feature. This PR does not generate constructs from a swagger/openapi spec - it relies on API Gateway's ability to do this during CloudFormation deployment.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license