Skip to content

Use ServiceExceptionSerializer for all NotImplementedError handling #13068

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Use ServiceExceptionSerializer for all NotImplementedError handling #13068

wants to merge 1 commit into from

Conversation

k-a-il
Copy link
Contributor

@k-a-il k-a-il commented Aug 28, 2025 •
edited
Loading

Motivation

We're adding custom error messages for cases where an AWS service or operation isn't implemented. While working on this we noticed that currently the NotImplementedError can be handled in two places: in the Skeleton class and in the ServiceExceptionSerializer. It doesn’t seem necessary to have it in both. This PR simplifies this logic by removing the error handling from Skeleton and using the existing logic in ServiceExceptionSerializer instead

Changes

  • Removed the NotImplementedError exception handling from Skeleton
  • Added analytics event to ServiceExceptionSerializer for the NotImplementedError exception

@k-a-il k-a-il self-assigned this Aug 28, 2025
@k-a-il k-a-il added semver: patch Non-breaking changes which can be included in patch releases skip-docs Pull request does not require documentation changes labels Aug 28, 2025
@k-a-il k-a-il changed the title Move NotImplementedError handling from Skeleton class to ServiceExceptionSerializer Use ServiceExceptionSerializer for all NotImplementedError handling Aug 28, 2025
@github-actions GitHub Actions
Copy link

S3 Image Test Results (AMD64 / ARM64)

    2 files  ±0    2 suites  ±0   8m 24s ⏱️ +30s
  517 tests ±0  467 ✅ ±0   50 💤 ±0  0 ❌ ±0 
1 034 runs  ±0  934 ✅ ±0  100 💤 ±0  0 ❌ ±0 

Results for commit 1bd72ea. ± Comparison against base commit 38aecc7.

@github-actions GitHub Actions
Copy link

Test Results - Preflight, Unit

22 158 tests  ±0   20 418 ✅  - 3   6m 29s ⏱️ -2s
     1 suites ±0    1 737 💤 ±0 
     1 files   ±0        3 ❌ +3 

For more details on these failures, see this check.

Results for commit 1bd72ea. ± Comparison against base commit 38aecc7.

@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 40m 57s ⏱️ - 2m 21s
4 632 tests ±0  4 189 ✅ ±0  443 💤 ±0  0 ❌ ±0 
4 634 runs  ±0  4 189 ✅ ±0  445 💤 ±0  0 ❌ ±0 

Results for commit 1bd72ea. ± Comparison against base commit 38aecc7.

@k-a-il
Copy link
Contributor Author

k-a-il commented Aug 28, 2025

Hi @thrau , could you take a look at this draft PR and share your thoughts on the proposed changes? The unit tests for the Skeleton class are currently failing, I plan to fix those and add new tests for this change later. For now, I'd just like to check whether you're generally ok with modifying the current logic :)

@k-a-il k-a-il added this to the 4.9 milestone Sep 4, 2025
@k-a-il k-a-il mentioned this pull request Sep 5, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thrau thrau Awaiting requested review from thrau thrau will be requested when the pull request is marked ready for review thrau is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@k-a-il k-a-il

Labels
semver: patch Non-breaking changes which can be included in patch releases skip-docs Pull request does not require documentation changes
Projects
None yet
Milestone
4.9
Development

Successfully merging this pull request may close these issues.
1 participant
@k-a-il