CFN: add more validations to intrinsic FnEquals#13217
Conversation
fad4290 to
8f1c4ef
Compare
Test Results - Preflight, Unit22 298 tests ±0 20 555 ✅ ±0 14m 58s ⏱️ - 1m 14s Results for commit 38c6319. ± Comparison against base commit 658d6fb. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 36m 12s ⏱️ - 1h 23m 16s Results for commit 38c6319. ± Comparison against base commit 658d6fb. This pull request removes 4228 tests.♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files ± 0 5 suites ±0 48m 51s ⏱️ - 1h 49m 36s Results for commit 38c6319. ± Comparison against base commit 658d6fb. This pull request removes 4578 tests.♻️ This comment has been updated with latest results. |
Test Results - Alternative Providers584 tests - 783 330 ✅ - 381 26m 4s ⏱️ - 13m 52s Results for commit 38c6319. ± Comparison against base commit 658d6fb. This pull request removes 783 tests.♻️ This comment has been updated with latest results. |
8f1c4ef to
b5f6aea
Compare
ffeb0eb to
840ba70
Compare
simonrw
left a comment
There was a problem hiding this comment.
I think the test construction (with unused condition) is a little weird, however it's still valid behaviour that we now have parity with!
| } | ||
| } | ||
| }, | ||
| "tests/aws/services/cloudformation/test_change_set_fn_split.py::TestValidations::test_validate_args_len": { |
There was a problem hiding this comment.
question: where did these snapshots come from? I guess they are left over from your split PR (#13244), can we remove them before merging?
There was a problem hiding this comment.
You're right. Nice catch 👍
| @skip_if_legacy_engine | ||
| def test_validate_equals_args_len(self, aws_client, snapshot): | ||
| template = { | ||
| "Conditions": {"ShouldDeploy": {"Fn::Equals": ["a"]}}, |
There was a problem hiding this comment.
TIL you can have a condition that's unused!
| # entities (parameters, mappings, conditions, etc.). Then compute the output fields; computing | ||
| # only the output fields would only result in the deployment logic of the referenced outputs | ||
| # being evaluated, hence enforce the visiting of all the resources first. | ||
| self.visit(node_template.conditions) |
There was a problem hiding this comment.
I guess this is only needed because in your test you have an unused condition, but in general this should not be required since resources would refer to the conditions which would cause the visiting process to visit the conditions, right?
I'm glad we have this extra parity with explicitly visiting the conditions (perhaps we should explicitly visit all nodes?) but it seems like a more contrived change than it needs to be.
There was a problem hiding this comment.
I'll leave this here for now. But I have an idea to discuss later.
Motivation
This PR adds a validation on the number of arguments the Fn::Equals receives. Some user are getting finding issues with the new CFn engine due to the lack of ValidationError messages.
Example:
Should be:
Changes
Testing