CFn: Improve stack set support #19
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request enhances CloudFormation StackSet support in LocalStack, focusing on improving compatibility with the latest upstream commit of the copilot-local repository. Key changes include:
- Updated
get_template_bodyfunction to support using previous stack templates - Expanded
StackSetentity to allow access to template bodies forStackInstances - Implemented merging of
StackInstanceparameter overrides withStackSetparameter definitions - Introduced
convert_in_place_at_jsonpathutility function for deep nested structure in-place type conversion - Enhanced
test_create_stack_set_with_stack_instancesto cover parameter overrides and multi-region deployment - Added support for looking up stack sets when retrieving template summaries
- Improved error handling for S3 URLs in template handling functions
These changes aim to address compatibility issues with the latest copilot-local repository and expand LocalStack's CloudFormation StackSet capabilities.
10 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings
| def get_template(self): | ||
| if body := self.template_body: | ||
| return body | ||
|
|
||
| raise NotImplementedError("template URL") |
There was a problem hiding this comment.
logic: Template URL handling is not implemented. This could lead to issues if users try to use template URLs.
| def get_instance(self, account: str, region: str) -> StackInstance | None: | ||
| for instance in self.stack_instances: | ||
| if instance.metadata["Account"] == account and instance.metadata["Region"] == region: | ||
| return instance | ||
| return None |
There was a problem hiding this comment.
style: Consider using a dictionary for faster lookup of stack instances instead of iterating through a list.
| def find_stack_set(account_id: str, region_name: str, stack_set_name: str) -> StackSet | None: | ||
| state = get_cloudformation_store(account_id, region_name) | ||
| for name, stack_set in state.stack_sets.items(): | ||
| # TODO: stack set id? | ||
| if stack_set_name in [name, stack_set.stack_set_name]: | ||
| return stack_set |
There was a problem hiding this comment.
style: Consider adding a docstring to explain the function's purpose and parameters
| state = get_cloudformation_store(account_id, region_name) | ||
| for name, stack_set in state.stack_sets.items(): | ||
| # TODO: stack set id? | ||
| if stack_set_name in [name, stack_set.stack_set_name]: |
There was a problem hiding this comment.
logic: This comparison might lead to unexpected matches if stack_set_name is a substring of name or stack_set_name
| if not old_value: | ||
| return |
There was a problem hiding this comment.
style: This check might skip valid falsy values (e.g., False, 0). Consider using 'is None' instead
Motivation
I updated our copilot-local repo to the latest upstream commit, and it did not work against LocalStack any more. I am not 100% sure the current commit works either.
The initial failure reason was our StackSet support in CloudFormation.
Changes
get_template_bodyto allow use of the previous stack (UsePreviousStack)StackSetentity mostly allowing access to the template body for use byStackInstancesStackInstanceparameter overrides withStackSetparameter definitionsconvert_in_place_at_jsonpathfor deep nested structure in place type conversiontest_create_stack_set_with_stack_instancestest to support parameter overridesTesting
I have been testing with my fork of copilot-local which has a few changes to the LocalStack fork of copilot:
localstackbranch has been rebased onto the latest tagged release (v1.33.3 at the time of writing)As of 2024-06-07 I had not yet completed the work required to fix copilot-local. I think I was close, but not there yet.