Add possibility to ignore specific unsupported resource types in CloudFormation #13496
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 8m 4s ⏱️ Results for commit 977e8dd. ♻️ This comment has been updated with latest results. |
Test Results - Alternative Providers583 tests 325 ✅ 17m 49s ⏱️ Results for commit 977e8dd. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 33m 45s ⏱️ Results for commit 977e8dd. ♻️ This comment has been updated with latest results. |
silv-io
added
the
semver: minor
|
Currently, only patch changes are allowed on main. Your PR labels (semver: minor, docs: needed, notes: needed) indicate that it cannot be merged into the main at this time. |
silv-io
added
docs: needed
silv-io
requested review from
dominikschubert,
pinzon,
simonrw and
thrau
as code owners
# 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
left a comment
simonrw
left a comment
There was a problem hiding this comment.
I have a few minor suggestions/nits but I would like the test to be updated to use direct boto calls where possible. This is for clarity and ease of understanding the test going forward.
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py
Show resolved
Hide resolved
| TITLE_MESSAGE = "Unsupported resources detected:" | ||
|
|
||
| def __init__(self): | ||
| def __init__(self, change_set_type: ChangeSetType | str | None = None): |
There was a problem hiding this comment.
nit: it would be nice to not need the str type here, and surely it's always known so surely None is not a valid type?
There was a problem hiding this comment.
Yeah you're right. I think in some place I defined it as a string once and then had curly lines below, which I then auto-fixed. Just setting ChangeSetType should work fine.
|
|
||
|
|
||
| def should_ignore_unsupported_resource_type( | ||
| resource_type: str, change_set_type: ChangeSetType | str | None |
There was a problem hiding this comment.
question: as stated above, under what circumstances is change_set_type either str or None?
tests/integration/test_catalog.py
Outdated
|
|
||
| # template with one supported and one unsupported resource | ||
| bucket_name = f"cfn-toggle-{short_uid()}" | ||
| template_body = textwrap.dedent( |
There was a problem hiding this comment.
suggestion: for inline templates like this, using a dictionary and rendering to a JSON string might be nicer
tests/integration/test_catalog.py
Outdated
| def _wait_for_complete_change_set(change_set_id: str): | ||
| def _describe(): | ||
| result = aws_client.cloudformation.describe_change_set(ChangeSetName=change_set_id) | ||
| status = result["Status"] | ||
| if status == "CREATE_COMPLETE": | ||
| return result | ||
| if status == "FAILED": | ||
| pytest.fail(f"change set unexpectedly failed: {result.get('StatusReason')}") | ||
| raise Exception("still waiting for change set") | ||
|
|
||
| return retry(_describe, retries=15, sleep=2) |
There was a problem hiding this comment.
suggestion: this function can be replaced by aws_client.cloudformation.get_waiter("change_set_create_complete").wait(...)
tests/integration/test_catalog.py
Outdated
| def test_ignore_unsupported_resources_toggle(testing_catalog, aws_client, monkeypatch, cleanups): | ||
| unsupported_resource = "AWS::LatestService::NotSupported" | ||
|
|
||
| def _create_change_set(stack_name: str, template_body: str): |
There was a problem hiding this comment.
nit I personally don't like this helper and would prefer the actual cfn function calls in the test, however I can see it's value as we perform this operation twice.
tests/integration/test_catalog.py
Outdated
| cs_id_ok, stack_id_ok = _create_change_set(stack_name_ok, template_body) | ||
| _wait_for_complete_change_set(cs_id_ok) | ||
| aws_client.cloudformation.execute_change_set(ChangeSetName=cs_id_ok) | ||
| _wait_for_stack_status(stack_name_ok, "CREATE_COMPLETE") |
There was a problem hiding this comment.
nit can we use the waiters here?
| _wait_for_stack_status(stack_name_ok, "CREATE_COMPLETE") | |
| aws_client.cloudformation.get_waiter("stack_create_complete").wait(StackName=stack_name_ok) |
tests/integration/test_catalog.py
Outdated
| monkeypatch.setattr(config, "CFN_IGNORE_UNSUPPORTED_TYPE_CREATE", [unsupported_resource]) | ||
| stack_name_ok = f"stack-ok-{short_uid()}" | ||
| cs_id_ok, stack_id_ok = _create_change_set(stack_name_ok, template_body) | ||
| _wait_for_complete_change_set(cs_id_ok) |
There was a problem hiding this comment.
| _wait_for_complete_change_set(cs_id_ok) | |
| aws_client.cloudformation.get_waiter("change_set_create_complete").wait(ChangeSetName=cs_id_ok) |
tests/integration/test_catalog.py
Outdated
| failed_cs = _wait_for_failed_change_set(cs_id_fail) | ||
| status_reason = failed_cs.get("StatusReason", "") | ||
| assert ChangeSetResourceSupportChecker.TITLE_MESSAGE in status_reason | ||
| assert unsupported_resource in status_reason |
There was a problem hiding this comment.
suggestion: then we don't need the inner function
| failed_cs = _wait_for_failed_change_set(cs_id_fail) | |
| status_reason = failed_cs.get("StatusReason", "") | |
| assert ChangeSetResourceSupportChecker.TITLE_MESSAGE in status_reason | |
| assert unsupported_resource in status_reason | |
| with pytest.raises(WaiterError) as exc_info: | |
| aws_client.cloudformation.get_waiter("change_set_create_complete").wait( | |
| ChangeSetName=cs_id_fail | |
| ) | |
| assert exc_info.value.last_response["Status"] == "FAILED" | |
| status_reason = exc_info.value.last_response["StatusReason"] | |
| assert ChangeSetResourceSupportChecker.TITLE_MESSAGE in status_reason | |
| assert unsupported_resource in status_reason |
|
|
||
|
|
||
| @markers.aws.only_localstack | ||
| def test_ignore_unsupported_resources_toggle(testing_catalog, aws_client, monkeypatch, cleanups): |
There was a problem hiding this comment.
issue: in general I would prefer not defining inner functions (especially if they only have one use), and to use the waiters where possible. I don't find them easy to use, and the waiters code is more straightforward to read in the test itself. For the successful cases we swap a function definition and call with a single line, and for the failure cases using the WaiterError is clearer IMO.
There was a problem hiding this comment.
thanks I didn't like it too much either but also didn't know about the waiters. Will switch it over :)
There was a problem hiding this comment.
I've fixed it and also went ahead and fixed the old test to use the waiters. It's much cleaner now :)
Motivation
As part of the IaC initiative we want to add the possibility to ignore only specific resource types when checking for their support.
Changes
should_ignore_unsupported_resource_typewhich is used in place of the general ignore of all unsupported resources in all locations.Tests
Related
Fixes FLC-84
Fixes FLC-198