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

add BodyS3Location property to AWS::ApiGateway::RestApi #8425

Merged
merged 4 commits into from Jun 5, 2023

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jun 2, 2023

Depends on #8371

Implementing the BodyS3Location property to AWS::ApiGateway::RestApi as requested by a customer.

This property allows Cloudformation to directly fetch the "Body" property from an S3 bucket.

However, I am not sure how to do negative testing with Cloudformation, so for now any S3 exception is going to bubble up?

Small note: I've also removed a line from the API Gateway importer because it was erasing tags after updating a RestAPI, which is not the default mode anymore.

Resource:
https://docs.aws.amazon.com/fr_fr/AWSCloudFormation/latest/UserGuide/aws-properties-apigateway-restapi-s3location.html

@bentsku bentsku self-assigned this Jun 2, 2023
@bentsku bentsku added aws:cloudformation AWS CloudFormation aws:apigateway Amazon API Gateway semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Jun 2, 2023
@bentsku bentsku force-pushed the cfn/add-restapi-s3bodylocation branch from 510c40b to 1faf6c5 Compare June 2, 2023 16:39
@bentsku bentsku changed the base branch from master to fix-apigw-import June 2, 2023 16:39
@bentsku bentsku changed the title wip: add S3BodyLocation property to AWS::ApiGateway::RestApi wip: add BodyS3Location property to AWS::ApiGateway::RestApi Jun 2, 2023
@bentsku bentsku marked this pull request as ready for review June 2, 2023 16:45
@bentsku bentsku requested a review from calvernaz as a code owner June 2, 2023 16:45
@bentsku bentsku changed the title wip: add BodyS3Location property to AWS::ApiGateway::RestApi add BodyS3Location property to AWS::ApiGateway::RestApi Jun 2, 2023
@bentsku bentsku force-pushed the cfn/add-restapi-s3bodylocation branch from 1faf6c5 to dd0649b Compare June 2, 2023 16:49
@bentsku bentsku marked this pull request as draft June 3, 2023 11:30
@bentsku bentsku force-pushed the cfn/add-restapi-s3bodylocation branch from dd0649b to 7cc6140 Compare June 4, 2023 00:54
@bentsku bentsku force-pushed the cfn/add-restapi-s3bodylocation branch from 7cc6140 to 81aa7f2 Compare June 4, 2023 01:32
@coveralls
Copy link

coveralls commented Jun 4, 2023

Coverage Status

coverage: 82.755% (+0.004%) from 82.751% when pulling f573e97 on cfn/add-restapi-s3bodylocation into c5ea3bf on fix-apigw-import.

@bentsku bentsku marked this pull request as ready for review June 4, 2023 08:41
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for extending this!

localstack/services/cloudformation/models/apigateway.py Outdated Show resolved Hide resolved
localstack/services/cloudformation/models/apigateway.py Outdated Show resolved Hide resolved
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for extending this!

@bentsku bentsku force-pushed the cfn/add-restapi-s3bodylocation branch from 81aa7f2 to f573e97 Compare June 5, 2023 16:29
Copy link
Contributor

@calvernaz calvernaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @bentsku

localstack/services/cloudformation/models/apigateway.py Outdated Show resolved Hide resolved
@@ -241,6 +241,8 @@ def update_rest_api(

@handler("PutRestApi", expand=False)
def put_rest_api(self, context: RequestContext, request: PutRestApiRequest) -> RestApi:
# TODO: take into account the mode: overwrite or merge
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, no "mode" means "merge" - not necessarily a fix for now, but we could return a non-supported error if the mode is overwrite

Copy link
Contributor Author

@bentsku bentsku Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but right now we are not merging but always overwriting I believe? we do the following in the importer:

# Remove default root, then add paths from API spec
rest_api.resources = {}

Base automatically changed from fix-apigw-import to master June 5, 2023 18:33
@bentsku bentsku force-pushed the cfn/add-restapi-s3bodylocation branch from f573e97 to df720b7 Compare June 5, 2023 18:37
@github-actions
Copy link

github-actions bot commented Jun 5, 2023

LocalStack Community integration with Pro

2 086 tests   1 805 ✔️  1h 21m 18s ⏱️
       2 suites     281 💤
       2 files           0

Results for commit df720b7.

@bentsku bentsku merged commit 061594d into master Jun 5, 2023
24 checks passed
@bentsku bentsku deleted the cfn/add-restapi-s3bodylocation branch June 5, 2023 20:04
@dfangl dfangl added this to the 2.2 milestone Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:apigateway Amazon API Gateway aws:cloudformation AWS CloudFormation semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants