Check in the lockfile

[jcoyne]

This prevents Rubocop from automatically using the latest version and breaking the builds. This also gives people a baseline where they can point at a set of dependencies that passed the build. This is the practice recommended by Bundler: https://bundler.io/guides/faq.html\#using-gemfiles-inside-gems

[jrochkind]

My understanding has always been that this is generally considered bad practice in gems. I am kind of shocked to see Bundler recommending differently.

Precisely because it will mean we won't be testing with the latest version of gems -- and won't be testing with the versions that someone installing Blacklight on any given day will be testing with.

All the CI would be on frozen versions, only updated if someone were to take the time to do a PR to check in an updated Gemfile.lock.

Based on my experience with Blacklight's development, as we all know somewhat under-resourced for the task before us, I would predict this will lead to frequent occasions where BL is broken based on newly released gem versions (allowed by BL gemspec), but which are not caught by CI. And will lead to people having errors caused by this where it isn't even obvious that it was caused by this, it will be mysterious.

Unless I'm misunderstanding the situation? If I am not, I am not in favor of this change.

I think it is positive to have BL CI running with the latest versions of dependencies that are allowed by the gemspec -- these are the same versions that anyone installing a fresh BL at the moment the CI runs would get, after all. (Or running bundle update at the moment the CI runs).

While checking lockfile into repo means BL maintainers won't be forced to deal with any breakages of dependencies as soon as they happen -- they still will be happening, the burden of diagnosing them and dealing with them is just pushed to implementors, and also actually made more complicated as it could be a long time after the breakage that it is actually noticed and diagnosed (we know that for instance few people are running BL8 in production -- if a dependency relesae breaks BL8 but CI is frozen, how long would it take for someone to notice the breakage (which may or may not effect every app) and diagnose it?)

[jcoyne]

All the CI would be on frozen versions, only updated if someone were to take the time to do a PR to check in an updated Gemfile.lock.

Yes. I would rather we don't have "I'm making a documentation change, why do I have to figure out why the build is broken?" concerns. We can set up something like dependabot to automatically update with new versions. Then you can see exactly which update breaks a build.

[jrochkind]

Actually, please note in bundler's recommendation this very important part in the last question:

The main advantage of not checking in your Gemfile.lock is that new checkouts (including CI) will immediately have failing tests if one of your dependencies changes in a breaking way. Instead of forcing every fresh checkout (and possible new contributor) to encounter broken builds, the Bundler team recommends either using a tool like Dependabot to automatically create a PR and run the test suite any time your dependencies release new versions. If you don’t want to use a dependency monitoring bot, we suggest creating an additional daily CI build that deletes the Gemfile.lock before running bundle install

We are not doing those things at present? If we were doing one of those things, committing gemfile.lock might be less disastrous -- although it's not clear to me the maintainance/complexity cost/benefit would actually be preferable?

But if a majority thinks it would be, I'd be less opposed to a PR that included one of those alternatives (or was subsequent to them), to ensure we have some CI build that does catch breakages. "an additional daily CI build that deletes the Gemfile.lock before running bundle install" and CI is not necessarily implausible.

But without those things, I think it would be disastrous -- and not actually following bundler's recommendations at all. But bundler's recommendations to commit lockfiles are predicated on using some other method to have CI quickly catch breakages due to new releases.

[jrochkind]

We can set up something like dependabot to automatically update with new versions.

Ok, I'd suggest we don't commit Gemfile.lock until we have done that?

Not sure if dependabot can do we need for free; the bundler suggestion to have a nightly build that runs without lockfile may be simpler and more feasible? I think it would be acceptable.

We'd have to actually... notice and respond to failures of that updated-deps build (or dependabot equivalent), which I'm not sure we will do, but I can see how that would still be preferable to not allowing any other PR's in the meantime.

[tpendragon]

I agree with @jrochkind.

I might be convincible if we had another CI entry that did a bundle update or something, so we'd at least know it was dependency problems, but I think these failing CI entries are the only thing keeping us working for new installs of Blacklight right now.

[jrochkind]

I'm def open to doing this WITH one (or personally I would prefer both?) of the measures the bundler docs recommend:

1. have dependabot automatically issue PR's to update the checked-in lockfile(s) with any newly released dependencies. I think it should be configured to do this for indirect dependencies too, because users newly installing (or bundle update'ing) will get the latest versions of those too. Dependabot is part of github now, and anything it can do is a free service

2. Have a regularly scheduled (I'd suggest weekly) github action that first deletes all lockfiles and/or runs bundle update before running CI, for another way of catching if latest dependencies are failing, thus someone doing a new install (or running bundle update) might run into breakages.

I started to take a look at seeing if I could help set up both of these things -- but the way Blacklight uses engine_cart as well as tests under multiple rails versions confused me and I wasn't sure how to proceed. (It's possible dependabot can't really work at all in this situation? If we can't do dependabot at all and can only do #2, I might increase frequency to nightly -- and make sure it's reporting to channel! Ideally even repeated messages every run -- we're just so likely to ignore these errors)

[jcoyne]

#3146 is an example of the build breaking unexpectedly, which this PR can prevent.

[jrochkind]

Totally. I'm for it if we can implement one or ideally both of the alternatives the bundler docs recommend.

When a new dependency release allowed by gemspec breaks teh build unexpectedly, we do want/need to find out about, and need to be encouraged to fix it. But also it would be great for it not to block other unrelated work, I get it.

[jcoyne]

We have currently hit another case where the main build has stopped working and I don't know why. Some dependency must have changed, but we currently have no good lockfile to work from in order to bisect changes.

Dependabot is perfectly capable of doing dependency updates.

[jrochkind]

I understand how that is challenging and frustrating, yeah.

While not a lockfile, if you have a green build still in Github Actions history, you can get a human-readable statement of exact versions used, that might be useful or better than nothing?

For instance, go to this green build for example, and open up the "Install dependencies" disclosure triangle. Would be annoying to try to construct a Gemfile.lock to use from that for git bisect, but probably hypothetically possible.

Also annoying but possibly better than nothing to just diff it to the output from a red build to have a human see what dependencies differ -- not input to git bisect, but still a way to try to debug. I have done that before.

As a practical matter thinking about it... how would a checked in lockfile work with CI that that runs under multiple versions of Rails (and ruby, and possibly other dependencies), when that is recorded in lockfile? Wouldn't CI have to erase the Gemfile.lock or alter it in some way anyway to get each matrix part to be correct, so the actual lockfile used in CI wouldn't be the one checked in anyhow? Or would we find a way to check in lockfiles for each build in the matrix (and an ergonomic way to update them all too I guess?)

[jrochkind]

The logs for this particular CI in this PR are no longer available, but I almost want to do an empty commit to understand how it's running CI under (eg) rails 6.1.7, when there's a checked in lockfile specifying 7.1.3. It seems like if it really is running CI under (eg) 6.1.7, it must not be using the exact lockfile as checked in, but either erasing it entirely or perhaps modifying it with a bundle install that modifies it?

[jrochkind]

I'm not sure if it does what you want (I understand you ideally want the solution in this PR), but if it would be helpful, it should be possible to get the actual Gemfile.lock created in CI to be preserved as a build artifact that could be used for bisecting (take Gemfile.lock from last green), assuming there's a green in the past 90 days before logs/artifacts are purged. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/storing-workflow-data-as-artifacts