Skip to content
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

fix: copy resource metadata before normalizing the information in it #5904

Merged
merged 4 commits into from
Sep 14, 2023

Conversation

mndeveci
Copy link
Contributor

@mndeveci mndeveci commented Sep 7, 2023

Which issue(s) does this change fix?

#5901

Why is this change necessary?

When using value references in yaml file, the objects that is been created are the same instance. For that reason, changing one value from one instance will also affect the other instances.

The behaviour above causes issues when updating/normalizing resource metadata information. With that, we are adding SamResourceId information, but if Metadata information is been shared between resources, then it updates for all of them, which creates issues down the road when we are extracting functions.

How does it address the issue?

This change copies metadata before starting changes, and assigns it back once it is finalized, so that it won't affect all of the references.

What side effects does this change have?

N/A

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mndeveci mndeveci marked this pull request as ready for review September 8, 2023 21:56
@mndeveci mndeveci requested a review from a team as a code owner September 8, 2023 21:56
@@ -456,6 +457,53 @@ def test_skip_normalizing_already_normalized_resource(self):
self.assertEqual("new path", template_data["Resources"]["Function1"]["Properties"]["Code"])
self.assertEqual("Function1", template_data["Resources"]["Function1"]["Metadata"]["SamResourceId"])

def test_referenced_data(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are we asserting based on the new changes?

Copy link
Contributor Author

@mndeveci mndeveci Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change, SamResourceId is assigned as FirstFunction for all resources due to the reason I explained in the PR desc.

After this change, all resources must have their logical id assigned into SamResourceId, we are asserting them at the end of this changed block.

There is a redundant function that I forgot to remove, please ignore that, will be removing it now.

@mndeveci mndeveci added this pull request to the merge queue Sep 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 14, 2023
@mndeveci mndeveci added this pull request to the merge queue Sep 14, 2023
Merged via the queue into aws:develop with commit 6136470 Sep 14, 2023
76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants