Conversation
|
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. |
Test Results - Alternative Providers587 tests 331 ✅ 18m 49s ⏱️ Results for commit 1b70e37. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 32m 55s ⏱️ Results for commit 1b70e37. ♻️ This comment has been updated with latest results. |
d56f88f to
d0fd050
Compare
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 8m 24s ⏱️ Results for commit 1b70e37. ♻️ This comment has been updated with latest results. |
simonrw
left a comment
There was a problem hiding this comment.
The blocking issue is moving the resource support checking to after the transformation
localstack-core/localstack/config.py
Outdated
| CFN_IGNORE_UNSUPPORTED_RESOURCE_TYPES = is_env_not_false("CFN_IGNORE_UNSUPPORTED_RESOURCE_TYPES") | ||
|
|
||
| # Comma-separated list of resource type names that CLoudFormation will ignore on stack create/update | ||
| CFN_IGNORE_UNSUPPORTED_TYPE_CREATE: list[str] = parse_comma_separated_list( |
There was a problem hiding this comment.
question: do we need to include DELETE operations as well?
There was a problem hiding this comment.
It's not part of the spec so that's why I didn't add that. Is there a case where we'd need to specifically ignore deletion of a resource that we didn't handle in the create/update?
Also, I think I'll delete the addition of these env vars here because they'd be part of a next iteration PR. So we can discuss that in that follow-up
tests/integration/test_catalog.py
Outdated
| try: | ||
| aws_client.cloudformation.delete_stack(StackName=stack_id) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
suggestion(minor): we should probably use cleanups here but of course we know the stack deployment should fail so it's not a huge concern.
There was a problem hiding this comment.
I had that in there while working on it in case something went wrong. I can switch it over to using cleanups instead
| ) | ||
| validator.validate() | ||
|
|
||
| if not config.CFN_IGNORE_UNSUPPORTED_RESOURCE_TYPES: |
There was a problem hiding this comment.
issue: we should apply this step after transformation, because the transformation process may add resources to the template.
There was a problem hiding this comment.
In our initial discussion I think it was mentioned to be "after the validations" and I wasn't sure if the transformations are part of that or not. Thank you for spotting that :)
| support_status = self._resource_support_status(resource_type) | ||
| if support_status == CloudFormationResourcesSupportAtRuntime.AVAILABLE: | ||
| pass | ||
| else: | ||
| failure_message = self._build_resource_failure_message( | ||
| resource_type, support_status | ||
| ) | ||
| self._resource_failure_messages[resource_type] = failure_message |
There was a problem hiding this comment.
suggestion(style): maybe a match statement?
There was a problem hiding this comment.
I initially thought I'll use it in multiple locations, that's why I had the dict. match statement makes more sense now though, I agree :)
| self, resource_type: str | ||
| ) -> AwsServicesSupportStatus | CfnResourceSupportStatus: | ||
| service_name = _get_service_name(resource_type) | ||
| return self.catalog.get_cloudformation_resource_status(resource_type, service_name, True) |
There was a problem hiding this comment.
question: how do we deal with CFn custom resources? I see this was part of #13027 so I don't have the context here. Do we only look at resources with the AWS:: prefix?
There was a problem hiding this comment.
Just skimmed through the code and I don't think we do. I can add in a cross check with what we've added in #13027 here to make it safe 👍🏼
cc62478 to
06cb0e8
Compare
# Conflicts: # localstack-core/localstack/config.py
63314cb to
ca9cf02
Compare
simonrw
left a comment
There was a problem hiding this comment.
Thanks for addressing my feedback. Looks great 👍
Motivation
This PR is part of the IaC usability initiative. In that initiative we want to make error messages for Cfn clearer. Adding which resources can't be added an why in the DescribeStack is a first step for this.
Changes
Tests
Added a location for catalog integration tests in
tests/integration/test_catalog.pyand also a parametrized test for all error cases intest_catalog_reports_unsupported_resources_in_stack_statusRelated
FLC-62