fix: add unique hash to cloned source to avoid conflict #1136
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Issue ----- When more than one Terraform resource points to the same git repository, when we clone the Source object, it does not create a new cloned Source object as the PullRequest ID is the same, the source name is the same, and we use only these two values to generate the name of the cloned resource. Solution -------- Add a short and deterministic hash to the name of the cloned Source object. Another thoughts ---------------- It's not necessary to get a deterministic unique string, it could be random as we save the result as SourceRef. I decided to use a short (first 10 bytes of a sha256 hash), because if in the future we have to regenerate the values somehow, then we don't have to care about migration, extra compatibility checks. I know it's not likely we need this, but generating a random number - doesn't yield shorter code. - still doesn't guarantee it will not generate a conflicting name, and knowing computers, I think it's even more likely (still unlikely) to generate the same random value if the two generation happens (nearly) at the same time. Additional changes ------------------ - Removed the hardcoded index-magic from poll_test. For some reasons it was not always in order. I don't know what changed tho. Notes ---- Compatibility: I was thinking about what happens with old resources, but after I spent some time on it, my conclusion is "they are not affected". We don't calculate names when we look up source objects for terraform objects. Their name is already saved as SourceRef. We generate this game only on newly created resources, therefore it does not break anything already in place. Fixes #923 References: - #923 Signed-off-by: Balazs Nadasdi <[email protected]>
yitsushi
changed the title
|
Currently testing this. I have 1 source being referenced by 2 TF resources. Making a change to the TF code in a PR, results in 2 planned TF resources (one for each TF resource) and 2 new sources: |
yiannistri
reviewed
Nov 21, 2023
internal/server/polling/poll_test.go
Outdated
| for idx, item := range tfList.Items[1:] { | ||
| expectToEqual(t, g, item.Name, fmt.Sprintf("%s-pr-%d", original.Name, idx+1)) | ||
| expectToEqual(t, g, item.Spec.SourceRef.Name, fmt.Sprintf("%s-source-pr-%d", original.Name, idx+1)) | ||
| for _, item := range tfList.Items[1:] { |
There was a problem hiding this comment.
Should this start from 0 or rather iterate through the whole slice? Since we skip the item with the same name below.
Signed-off-by: Balazs Nadasdi <[email protected]>
yiannistri
approved these changes
Nov 21, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
When more than one Terraform resource points to the same git repository, when we clone the Source object, it does not create a new cloned Source object as the PullRequest ID is the same, the source name is the same, and we use only these two values to generate the name of the cloned resource.
Solution
Add a short and deterministic hash to the name of the cloned Source object.
Another thoughts
It's not necessary to get a deterministic unique string, it could be random as we save the result as SourceRef. I decided to use a short (first 10 bytes of a sha256 hash), because if in the future we have to regenerate the values somehow, then we don't have to care about migration, extra compatibility checks. I know it's not likely we need this, but generating a random number
Additional changes
Notes
Compatibility:
I was thinking about what happens with old resources, but after I spent some time on it, my conclusion is "they are not affected". We don't calculate names when we look up source objects for terraform objects. Their name is already saved as SourceRef. We generate this game only on newly created resources, therefore it does not break anything already in place.
Fixes #923
References: