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

remove delete integration patch #6074

Merged
merged 4 commits into from May 18, 2022
Merged

Conversation

calvernaz
Copy link
Contributor

@calvernaz calvernaz commented May 17, 2022

This PR removes the patch for the delete integration operation.

@calvernaz calvernaz marked this pull request as draft May 17, 2022
@calvernaz calvernaz had a problem deploying to localstack-ext-tests May 17, 2022 Failure
@github-actions
Copy link

@github-actions github-actions bot commented May 17, 2022

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   55m 27s ⏱️ - 4m 40s
1 034 tests ±0  1 006 ✔️ ±0  28 💤 ±0  0 ±0 
1 330 runs  ±0  1 275 ✔️ ±0  55 💤 ±0  0 ±0 

Results for commit 07e0b57. ± Comparison against base commit 1378863.

♻️ This comment has been updated with latest results.

@calvernaz calvernaz force-pushed the remove-delete-integration-patch branch from 40cfa5e to 428f06e Compare May 18, 2022
@calvernaz calvernaz had a problem deploying to localstack-ext-tests May 18, 2022 Failure
@calvernaz calvernaz had a problem deploying to localstack-ext-tests May 18, 2022 Failure
@calvernaz calvernaz force-pushed the remove-delete-integration-patch branch from 0402ea2 to 4917a42 Compare May 18, 2022
@calvernaz calvernaz temporarily deployed to localstack-ext-tests May 18, 2022 Inactive
@calvernaz calvernaz marked this pull request as ready for review May 18, 2022
@calvernaz calvernaz requested review from whummer and thrau May 18, 2022
Copy link
Member

@whummer whummer left a comment

LGTM overall @calvernaz ! 👍 Just a minor clarification/suggestion for simplification..

localstack/services/apigateway/provider.py Outdated Show resolved Hide resolved
@calvernaz calvernaz temporarily deployed to localstack-ext-tests May 18, 2022 Inactive
thrau
thrau approved these changes May 18, 2022
Copy link
Member

@thrau thrau left a comment

LGTM! just a minor nitpick in the exception handling

except Exception as e:
raise NotFoundException("Invalid Resource identifier specified") from e
Copy link
Member

@thrau thrau May 18, 2022

Choose a reason for hiding this comment

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

It would make sense to limit the exception clause a bit. You can inspect botocore exceptions by catching CommonServiceException and then checking e.code for the particular error code.

Copy link
Contributor Author

@calvernaz calvernaz May 18, 2022

Choose a reason for hiding this comment

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

Do you have an example of that exception handle practice?

Copy link
Member

@thrau thrau May 18, 2022

Choose a reason for hiding this comment

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

in most cases you can simply pass through the exception and don't need to do anything. if moto raises a NotFoundException, then call_moto will re-raise something that is equivalent and will be rendered correctly.

here's an example where we inspect the exception and re-raise them conditionally. not sure that's your use case though

@handler("UpdateTable", expand=False)
def update_table(
self, context: RequestContext, update_table_input: UpdateTableInput
) -> UpdateTableOutput:
try:
# forward request to backend
result = self.forward_request(context)
except CommonServiceException as e:
is_no_update_error = (
e.code == "ValidationException" and "Nothing to update" in e.message
)
if not is_no_update_error or not list(
{"TableClass", "ReplicaUpdates"} & set(update_table_input.keys())
):
raise

Copy link
Contributor Author

@calvernaz calvernaz May 18, 2022

Choose a reason for hiding this comment

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

I guess it makes sense depending on the case, as we talked in private is not applicable in this case.

localstack/services/apigateway/patches.py Show resolved Hide resolved
@calvernaz calvernaz merged commit b296a99 into master May 18, 2022
17 checks passed
@calvernaz calvernaz deleted the remove-delete-integration-patch branch May 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants