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

move import_rest_api patch into ASF handler #6048

Merged
merged 4 commits into from May 16, 2022

Conversation

calvernaz
Copy link
Contributor

@calvernaz calvernaz commented May 11, 2022

Small PR to move the import_rest_api operation to the ASF handler. Doing that, we can remove our internal monkey patching.

@calvernaz calvernaz temporarily deployed to localstack-ext-tests May 11, 2022 20:41 Inactive
@calvernaz calvernaz marked this pull request as draft May 11, 2022 20:41
@github-actions
Copy link

github-actions bot commented May 11, 2022

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 14m 44s ⏱️ + 12m 26s
1 012 tests ±0     971 ✔️ ±0  41 💤 ±0  0 ±0 
1 308 runs  ±0  1 240 ✔️ ±0  68 💤 ±0  0 ±0 

Results for commit 10253a9. ± Comparison against base commit c31d245.

♻️ This comment has been updated with latest results.

@calvernaz calvernaz temporarily deployed to localstack-ext-tests May 11, 2022 22:04 Inactive
@calvernaz calvernaz temporarily deployed to localstack-ext-tests May 12, 2022 08:29 Inactive
@calvernaz calvernaz requested review from thrau and whummer May 12, 2022 09:16
@calvernaz calvernaz marked this pull request as ready for review May 12, 2022 09:19
Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

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

Nice, great to see the patch being removed and moved into the provider instead! 💪 Would propose to move the util function into moto.py, but will let @thrau do a more thorough review.. 👍


Ripped :call_moto_with_request: from moto.py but applicable to any operation (operation_name).
"""
local_context = create_aws_request_context(
Copy link
Member

Choose a reason for hiding this comment

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

Cool! Looks quite useful - since this is generally applicable, I'd probably move this to moto.py. (as a generalization of call_moto_with_request, which can then call this function with operation_name=context.operation.name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this was result of a conversation with @thrau, we don't necessarilly think this will be the pattern to use, hence kept it close where it's used. Moving it to the moto.py is not problem for me, but might result in being spread unnecessarily?

"PutRestApi",
PutRestApiRequest(
restApiId=response.get("id"),
failOnWarnings=str_to_bool(fail_on_warnings) or False,
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity - can the runtime type of fail_on_warnings be str here, did you actually observe this behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a string and easy observable running the following command.

awslocal apigateway import-rest-api --cli-binary-format raw-in-base64-out --fail-on-warnings --body 'file:///spec.json

Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

LGTM!

I'm OK with merging the PR as is if we leave _call_moto closed in the module. A generalization of the way it is now is not great imo. It's not clear from the interface what exactly from the request context that is being passed to _call_moto is being re-used (and why i would want that in the first place). We're passing down headers from the original request that may not belong to the subsequent or may even cause parser errors if passing unexpected headers.

@calvernaz calvernaz merged commit c2c9ed6 into master May 16, 2022
@calvernaz calvernaz deleted the remove-import-rest-api-patch branch May 16, 2022 21:07
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants