Skip to content

CFn: validate Type is provided #13524

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
Merged
simonrw merged 4 commits into from
Dec 22, 2025
Merged

CFn: validate Type is provided #13524

simonrw merged 4 commits into from
Dec 22, 2025

Conversation

@simonrw
Copy link
Contributor

@simonrw simonrw commented Dec 15, 2025 •
edited
Loading

Motivation

While discusing #13496 I asserted that we can safely access resource["Type"] in the visitors since we validate that the Type key is always present. However this was not true! This PR resolves this

Changes

  • Add test deploying resource without a Type key
  • Validate that the type is present during the validation phase

@simonrw simonrw requested a review from pinzon as a code owner December 15, 2025 11:04
@simonrw simonrw added semver: patch Non-breaking changes which can be included in patch releases docs: skip Pull request does not require documentation changes labels Dec 15, 2025
@simonrw simonrw added notes: skip Pull request does not have to be mentioned in the release notes semver: patch Non-breaking changes which can be included in patch releases docs: skip Pull request does not require documentation changes labels Dec 15, 2025
@github-actions
Copy link

github-actions bot commented Dec 15, 2025 •
edited
Loading

Test Results - Preflight, Unit

23 046 tests   21 201 ✅  6m 25s ⏱️
     1 suites   1 845 💤
     1 files         0 ❌

Results for commit a328b78.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Dec 15, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests   5 ✅  2m 57s ⏱️
1 suites  2 💤
1 files    0 ❌

Results for commit a328b78.

♻️ This comment has been updated with latest results.

@simonrw simonrw mentioned this pull request Dec 15, 2025
Merged
@github-actions
Copy link

github-actions bot commented Dec 15, 2025 •
edited
Loading

LocalStack Community integration with Pro

  2 files    2 suites   29m 36s ⏱️
585 tests 472 ✅ 113 💤 0 ❌
587 runs  472 ✅ 115 💤 0 ❌

Results for commit a328b78.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Dec 15, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

  5 files    5 suites   42m 33s ⏱️
609 tests 497 ✅ 112 💤 0 ❌
615 runs  497 ✅ 118 💤 0 ❌

Results for commit a328b78.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Dec 15, 2025 •
edited
Loading

Test Results - Alternative Providers

584 tests   325 ✅  18m 5s ⏱️
  1 suites  259 💤
  1 files      0 ❌

Results for commit a328b78.

♻️ This comment has been updated with latest results.

pinzon
pinzon requested changes Dec 22, 2025
View reviewed changes
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_validator.py
Comment on lines +166 to +169
if is_nothing(node_resource.type_.value):
raise ValidationError(
f"Template format error: [{node_resource.scope}] Every Resources object must contain a Type member."
)
Copy link
Member

@pinzon pinzon Dec 22, 2025

Choose a reason for hiding this comment

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

Why is this validation not added to the Preproc like the rest?

Copy link
Contributor Author

@simonrw simonrw Dec 22, 2025

Choose a reason for hiding this comment

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

Because it must operate on the transformed template and if we implement this in the preproc then it will trigger on the pre transformed template if it has eg a transform like include.

Copy link
Contributor Author

@simonrw simonrw Dec 22, 2025

Choose a reason for hiding this comment

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

See that ci is failing now after making your suggestion

@simonrw
CFn: validate Type is provided
b6af98b 
# Motivation

While discusing #13496 I asserted that we can safely access `resource["Type"]` in the visitors since we validate that the `Type` key is always present. However this _was not true_! This PR resolves this

# Changes

* Add test deploying resource without a `Type` key
* Validate that the type is present during the modelling phase
@simonrw
Move validation to validator
b07096c
@simonrw
Move validation to preproc
32fc1e3
@simonrw simonrw requested a review from pinzon December 22, 2025 19:51
pinzon
pinzon approved these changes Dec 22, 2025
View reviewed changes
Copy link
Member

@pinzon pinzon left a comment

Choose a reason for hiding this comment

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

Thanks for testing the suggestion 👍

@simonrw simonrw merged commit cfefefc into main Dec 22, 2025
43 checks passed
@simonrw simonrw deleted the cfn/required-types branch December 22, 2025 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pinzon pinzon pinzon approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

Assignees

No one assigned

Labels

docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@simonrw @pinzon