-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
feat(cloudformation): SAM Globals support with CloudFormation #6657
Conversation
checkov/cloudformation/cfn_utils.py
Outdated
@@ -1,5 +1,6 @@ | |||
from __future__ import annotations | |||
|
|||
from copy import deepcopy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have the pickle_deepcopy
func for performance wise :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great I'll use it, Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and well tested 💯
checkov/cloudformation/cfn_utils.py
Outdated
return new_template # Return the new template even if there were no globals to apply | ||
|
||
|
||
def deep_merge(dict1: DictNode, dict2: DictNode) -> DictNode: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this function can be moved under DictNode
class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
@@ -154,10 +154,16 @@ def _add_sam_globals(self) -> None: | |||
transform_step=True, | |||
) | |||
elif isinstance(value, list): | |||
# Remove duplicates | |||
new_value = [*vertex.attributes[property], *value] | |||
new_value_unique = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use list(set(new_value))
.
also new_value
is a bad name, I would call it list_updated_value
or something like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the lists there have multiple types in them so list(set(new_value))
didn't work. And I'll change the name
enriched_template = enrich_resources_with_globals(original_template) | ||
self.assertEqual(enriched_template, expected_template) | ||
|
||
def test_deep_merge_non_conflicting(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice tests!
…crewio#6657) * Enrich resources with globals
…crewio#6657) * Enrich resources with globals
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Added partial support for configuring Globals section according to SAM syntax
Fixes #2758
Fix
SAM properties that are represented in the same data structure as in CloudFormation syntax are merged. Other properties (such as Tags) are not merged as part of this feature.
Checklist: