-
Notifications
You must be signed in to change notification settings - Fork 249
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(aws-openapigateway-lambda): update construct to allow specifying an inline api definition #1190
feat(aws-openapigateway-lambda): update construct to allow specifying an inline api definition #1190
Conversation
… an inline api definition The rationale for this change is to improve the local developer experience and enable compatibility with `sam local start-api`. Previously, the construct only supported referencing an external OpenAPI definition file, which made it challenging to work with an API locally during development.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thanks - we'll take a look! |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
if (!apiDefinitionInline) { | ||
// This custom resource will overwrite the string placeholders in the openapi definition with the resolved values of the lambda URIs | ||
apiDefinitionWriter = resources.createTemplateWriterCustomResource(this, 'Api', { | ||
// CheckAlbProps() has confirmed the existence of these values |
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.
CheckOpenapiProps(), not alb
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.
Good catch - I've amended.
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 |
apiDefinitionWriter.s3Key | ||
); | ||
} else if (apiRawInlineSpec) { | ||
const apiInlineSpec = new apigateway.InlineApiDefinition(apiRawInlineSpec); |
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.
Perhaps this isn't needed since apiRawInlineSpec
is technically already expected to be a JSON object. Unless you are relying on the initial assignment to trigger validation, which would make sense in this instance. Non-and empty objects would throw.
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.
Yep, code is redundant to the check already done in CheckOpenapiProps. But including it makes the code explicit, these steps aren't just executing because it is "not something else", it is executing because it IS "this". Putting that in code is better than a comment - even at the cost of a couple clock ticks.
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 |
@@ -126,6 +126,7 @@ new OpenApiGatewayToLambda(this, "OpenApiGatewayToLambda", OpenApiGatewayToLambd | |||
|apiDefinitionBucket?|[`s3.IBucket`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_s3.IBucket.html)|S3 Bucket where the OpenAPI spec file is located. When specifying this property, `apiDefinitionKey` must also be specified.| | |||
|apiDefinitionKey?|`string`|S3 Object name of the OpenAPI spec file. When specifying this property, `apiDefinitionBucket` must also be specified.| | |||
|apiDefinitionAsset?|[`aws_s3_assets.Asset`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_s3_assets.Asset.html)|Local file asset of the OpenAPI spec file.| | |||
|apiJsonDefinition?|any|OpenAPI specification represented in a JSON object to be included in the CloudFormation template. IMPORTANT - Including the spec in the template introduces a risk of the template growing too big, but there are some use cases that require an embedded spec. Unless your use case explicitly requires an embedded spec you should pass your spec as an S3 asset.| |
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 just a nit, that the naming convention for the other api definition properties followed apiDefinition{thing}
.
import { ApiIntegration, CheckOpenapiProps, TokenToFunctionMapping, ObtainApiDefinition } from './openapi-helper'; | ||
export { ApiIntegration, TokenToFunctionMapping as ApiLambdaFunction } from './openapi-helper'; |
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.
As we don't normally export imports? in this library, could we add a comment here explaining the why and how its used?
@@ -77,6 +36,13 @@ export interface OpenApiGatewayToLambdaProps { | |||
* Local file asset of the OpenAPI spec file. | |||
*/ | |||
readonly apiDefinitionAsset?: Asset; | |||
/** | |||
* The OpenApi spec represented as a json object to be included in the CloudFormation template. |
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 AS A JSON OBJECT
bit of this is important, as a yaml-defined openapi spec wouldn't work, though is probably the more common format to define it. How can we better call that out in this comment to make sure customers know?
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.
To me the key point is that this definition will be embedded in the template, so the comment is focused on that. The README is slightly different, do you like that any better?
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.
Not passing JSON would cause this to die at synth time, right? I think its fine.
/** | ||
* @internal This is an internal core function and should not be called directly by Solutions Constructs clients. | ||
*/ | ||
export function CheckOpenapiProps(props: OpenApiProps) { |
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.
Nit - CheckOpenApiProps
to match the casing of OpenApi
elsewhere in this code.
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.
looks good to me
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 |
52fbcad
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #, if available:
Description of changes:
The rationale for this change is to improve the local developer experience and enable compatibility with
sam local start-api
. Previously, the construct only supported referencing an external OpenAPI definition file, which made it challenging to work with an API locally during development.Example Usage