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

Adopt CacheDiff (crate) over MetadataDiff (internal) #370

Merged
merged 21 commits into from
Dec 13, 2024

Conversation

schneems
Copy link
Contributor

The MetadataDiff was a prototype for how to extract cache clearing logic to a centralized trait. From there I changed the name and built a proc-macro that can generate the same logic https://github.com/heroku-buildpacks/cache_diff. The benefit of the proc macro is it works with defaults out of the box and adds consistency.

This PR moves the old code using MetadataDiff to now use CacheDiff. In the process I refactored some logic, namely how the OS distribution information is stored as metadata. Commits are small, sequential and self described.

The CacheDiff is a public crate while MetadataDiff was a temporary experimentation platform internal to this buildpack
The idea is to centralize all OS distribution information into a single attribute.

It's not yet used.
The coming move to CacheDiff prefers if one attribute relates to one difference reason. V2 introduced a distribution name and distribution value which we later determined looked better if it's combined to look like a single attribute to the user "OS Distribution." This move brings the attributes inline with how they're compared, now both name and value are evaluated and emited as a single unit.
The coming move to CacheDiff prefers if one attribute relates to one difference reason. V2 introduced a distribution name and distribution value which we later determined looked better if it's combined to look like a single attribute to the user "OS Distribution." This move brings the attributes inline with how they're compared, now both name and value are evaluated and emitted as a single unit.
Does not compile yet due to tests
@schneems schneems force-pushed the schneems/pub-traits branch from ed35f5c to aaf7087 Compare December 12, 2024 22:20
In the refactor we had two `diff` methods, now there's only one so we can use it instead of having to reference the trait. This shaves off some lines of the total diff.
@schneems schneems force-pushed the schneems/pub-traits branch from aaf7087 to fd11c67 Compare December 12, 2024 22:27
@schneems schneems marked this pull request as ready for review December 12, 2024 22:37
@schneems schneems requested a review from a team as a code owner December 12, 2024 22:37
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

(This was mainly a rubber stamp given it's a refactor and compiler/tests should cover this; let me know if you'd prefer a full review :-) )

@schneems schneems merged commit d2fd043 into main Dec 13, 2024
6 checks passed
@schneems schneems deleted the schneems/pub-traits branch December 13, 2024 15:47
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.

2 participants