-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
add support for Exports/Imports in CFn v2 #12906
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
Conversation
LocalStack Community integration with Pro 2 files 2 suites 22m 29s ⏱️ Results for commit d33106a. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 33m 18s ⏱️ Results for commit d33106a. ♻️ This comment has been updated with latest results. |
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.
Thanks for adding these changes. The blocker is filtering the stacks by status. Perhaps we can add a test for this case as well, i.e. create two stacks with exports, delete one stack then call list_exports
?
localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_preproc.py
Outdated
Show resolved
Hide resolved
42c4e41
to
2e34cb0
Compare
Thanks for the comments. after implementing the changes the feature code feels much better 👍 |
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.
Thanks for taking another stab at this, I think we have to move the behaviour of the ImportValue
implementation to the executor, and I really think we should make the exports in the store consistent with other fields there (add a _v2
dictionary).
def visit_node_intrinsic_function_fn_import_value( | ||
self, node_intrinsic_function: NodeIntrinsicFunction | ||
) -> PreprocEntityDelta: | ||
def _compute_fn_import_value(string) -> str: | ||
if not isinstance(string, str): | ||
raise RuntimeError(f"Invalid parameter for import: '{string}'") | ||
|
||
exports = exports_map( | ||
account_id=self._change_set.account_id, region_name=self._change_set.region_name | ||
) | ||
if not exports.get(string): | ||
raise RuntimeError(f"Value not found for import: '{string}'") | ||
|
||
return exports.get(string)["Value"] | ||
|
||
arguments_delta = self.visit(node_intrinsic_function.arguments) | ||
delta = self._cached_apply( | ||
scope=node_intrinsic_function.scope, | ||
arguments_delta=arguments_delta, | ||
resolver=_compute_fn_import_value, | ||
) | ||
return delta | ||
|
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.
issue: I think this lookup happens only in the executor. For example when describing a change set, the import is not retrieved.
I can reproduce this by
- creating a change set including an
ImportValue
, e.g.
Resources:
MyFoo:
Type: AWS::SSM::Parameter
Properties:
Type: String
Value: !ImportValue MyGlobalExportValue
- describing the change set with
aws describe-change-set ... --include-property-values
On AWS theValue
field isKNOWN_AFTER_APPLY
, but on LocalStack this code crashes because the export is fetched (but doesn't exist).
Can we add a test to cover this behaviour and implement the correct behaviour in the executor only.
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.
I added a test and it passes with the functionality in the Preproc. maybe I'm not understanding correctly the issue.
2e34cb0
to
d7de3b3
Compare
Currently, only patch changes are allowed on main. Your PR labels (semver: minor) indicate that it cannot be merged into the main at this time. |
b0acf1b
to
3cce2cb
Compare
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.
I have reviewed the tests again and think we are missing a scenario that I encountered that this test does not cover. It also explains my reasoning in this message: what if the export does not exist?
I had deployed a stack and the export did not exist. The change set was still created but there were the following differences:
- in the
Changes
array the after context was a stringified JSON object including{{changeSet:KNOWN_AFTER_APPLY}}
for the value
"Changes": [
{
"Type": "Resource",
"ResourceChange": {
"Action": "Add",
"LogicalResourceId": "MyFoo",
"ResourceType": "AWS::SSM::Parameter",
"Scope": [],
"Details": [],
"AfterContext": "{\"Properties\":{\"Value\":\"{{changeSet:KNOWN_AFTER_APPLY}}\",\"Type\":\"String\"}}"
}
}
],
- The
StatusReason
contained the following message:[WARN] --include-property-values option can return incomplete ChangeSet data because: ChangeSet creation failed for resource [MyFoo] because: No export named MyGlobalExportValue
Actually there is another test case that would be interesting:
- create change set that imports from an export that does not exist
- deploy the stack with the export
- execute the first change set
I don't think we need 100% parity here, but it's certainly an interesting case.
So in summary:
- it's ok to create and describe a change set that imports values that have not been exported
- it's not ok to execute a change set that references exports that don't exist
- it's possible imports are looked up both during the modelling phase and execution phase, depending on the results of adding my second test suggestion
tests/aws/services/cloudformation/test_change_sets_exports_imports.py
Outdated
Show resolved
Hide resolved
tests/aws/services/cloudformation/test_change_sets_exports_imports.py
Outdated
Show resolved
Hide resolved
tests/aws/services/cloudformation/test_change_sets_exports_imports.py
Outdated
Show resolved
Hide resolved
tests/aws/services/cloudformation/test_change_sets_exports_imports.py
Outdated
Show resolved
Hide resolved
tests/aws/services/cloudformation/test_change_sets_exports_imports.py
Outdated
Show resolved
Hide resolved
@pinzon I have added a commit that includes both scenarios I was thinking of. Now we have coverage of:
|
Motivation
This PR enables the functionality of Exports/Imports in CFn v2
Changes
Testing
Todo