Skip to content

Add resource property validation framework to SAM translator. #3805

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
Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Add resource property validation framework to SAM translator. #3805

wants to merge 4 commits into from

Conversation

vicheey
Copy link
Contributor

@vicheey vicheey commented Aug 22, 2025

Issue #, if available

None

Description of changes

This PR introduces a comprehensive validation framework for SAM resource properties, and enhancing the translator's ability to catch configuration errors early in the deployment process.

  • Enhanced validation logic in samtranslator/model/init.py
  • Updated SAM resources in samtranslator/model/sam_resources.py to integrate
    validation
  • Comprehensive test coverage in tests/test_model.py

Description of how you validated changes

  • Added extensive test suite covering validation scenarios
  • All existing tests continue to pass

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@vicheey vicheey requested a review from a team as a code owner August 22, 2025 21:16
@vicheey
Copy link
Contributor Author

vicheey commented Aug 22, 2025

The Tests failed because of lower coverage percentage. (94.82% instead of 95%). Added validation logic push the overall total line increase and lower the coverage percentage.

Low coverage files:
• samtranslator/validator/validator.py - 27% coverage
• samtranslator/model/init.py - 69% coverage
• samtranslator/model/naming.py - 71% coverage
• samtranslator/region_configuration.py - 79% coverage

Removing deprecated and unused samtranslator/validator/validator.py

@vicheey
Copy link
Contributor Author

vicheey commented Aug 22, 2025 •
edited
Loading

Current usage analysis:
• No active imports of SamTemplateValidator in any source
files
• No test files directly testing SamTemplateValidator

What IS being used:
• samtranslator.validator.value_validator.sam_expect - Used extensively across 12+ files
• Direct jsonschema.validate() calls in tests (instead of the deprecated SAM wrapper)

However, we cannot delete this file yet because this public facing interface might be used by package consuming this package.
I added the samtranslator.validator.validator.py to test coverage ignore list.

--------------------------------
TOTAL   9640    314   3402    220    95%

Required test coverage of 95% reached. Total coverage: 95.28%

=============== 4118 passed in 86.15s (0:01:26) ================

@vicheey vicheey force-pushed the validation-framework-extraction branch from 188db2a to b52c322 Compare August 22, 2025 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnusunny bnusunny Awaiting requested review from bnusunny

@roger-zhangg roger-zhangg Awaiting requested review from roger-zhangg

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@vicheey