-
Notifications
You must be signed in to change notification settings - Fork 296
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
Ensure compatibility of code and git cache entries #704
base: main
Are you sure you want to change the base?
Conversation
Some code changes in omnibus can be incompatible with previously existing git cache entries, such that builds are incorrect or fail if the older git cache snapshot is restored in a build that is run after upgrading omnibus. Adding a serial number to the git cache allows omnibus to determine whether a git cache snapshot is compatible with the current version of omnibus in use.
👍 |
A couple of questions/thoughts for discussion:
Other than those point - looks great, thanks! @chef/engineering-services |
# will not have the generated content, so these snapshots would be | ||
# incompatible with the current omnibus codebase. Incrementing the serial | ||
# number ensures these old shapshots will not be used in subsequent builds. | ||
SERIAL_NUMBER = 1 |
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.
As the git caching is a hard to understand beast, I'm worried contributors won't know the appropriate time to bump this number. Would it make more sense to base the cache busting off the MAJOR.MINOR
of Omnibus::VERSION
?
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 think the downside of MAJOR.MINOR
is it does nothing for teams that ride master, unless we start releasing omnibus with every change (which is a possibility, I suppose). Would it be possible to use git sha?
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.
See my response below.
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'll elaborate.
If you use major.minor
, then:
- You can have false negatives, e.g., you can do a bug fix that looks fine but is actually incompatible with existing caches, and builds break.
- You will have false positives, e.g., a new feature (like support for a new platform) is added that doesn't affect cache validity at all, but resets your cache anyway.
If you use the SHA of the omnibus code then:
- You get a lot of false positives.
- You never get a false negative
Given the problems that clean caches can cause for us, we want to make the number of false positives as low as possible, so that makes the SHA-based option less attractive.
The idea is that you only clear the cache when you really need to. The downside is that you might forget to increment the serial number and then builds fail and you go back and do it. But many projects have really long builds and the consensus is that the tradeoffs of clearing the cache less frequently are worth it.
Not really. If the code changes such that the old caches are not valid with the new code, you're just gonna have bad builds. Your options are to not upgrade omnibus, or accept that the new code is not compatible with old caches. |
Also, the appveyor thing is just because source forge is down. Tests pass otherwise. |
👍 |
Description
Some code changes in omnibus can be incompatible with previously
existing git cache entries, such that builds are incorrect or fail if
the older git cache snapshot is restored in a build that is run after
upgrading omnibus. Adding a serial number to the git cache allows
omnibus to determine whether a git cache snapshot is compatible with the
current version of omnibus in use.
Note: merging this change will effectively clear the cache for every project once the project upgrades.
/cc @chef/omnibus-maintainers <- This ensures the Omnibus Maintainers team are notified to review this PR.