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

Made template diffing a bit smarter. #11378

Closed
wants to merge 1 commit into from

Conversation

apollo13
Copy link
Contributor

Previously templates diff always showed up as delete & add. The new code
always diffs if there is only one template and if there are multiple
templates it uses DestPath as unique identifier (this works because
nomad checks that this is actually unique).

@lgfa29 This is similar to the service diffing PR of mine that you reviewed. But this one is massively easier since DestPath has to be uniqe.

Previously templates diff always showed up as delete & add. The new code
always diffs if there is only one template and if there are multiple
templates it uses DestPath as unique identifier (this works because
nomad checks that this is actually unique).
@lgfa29
Copy link
Contributor

lgfa29 commented Oct 26, 2021

Nice! Thanks for catching this 🙂

Looking at the new code and the previous code, they both seem very similar, so I thought that maybe the issue is that the current primitiveObjectSetDiff function is hard coded to always use the object hash. But as you've correctly pointed out, there are some blocks that actually have some kind of unique ID.

So I prototyped this approach where the function can receive another helper function that returns that unique ID value for each object: 6930f37

I added your updated test case to that commit as well. I'm waiting on the full CI to run to make sure I didn't break anything, but at least the TestTaskDiff passed locally 😄

This approach could also help us improve other blocks, like artifact, which I suspect suffers from the same issue.

What do you think of this approach?

@lgfa29 lgfa29 self-assigned this Oct 26, 2021
@apollo13
Copy link
Contributor Author

apollo13 commented Oct 27, 2021 via email

@lgfa29
Copy link
Contributor

lgfa29 commented Oct 27, 2021

Cool, since you don't mind I will close this and raise a new one. Last time I caused a bit of confusion by approving and merging my own code 😅

Thanks so much for raising this!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants