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.

[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]

https://github.com/projectblacklight/blacklight/pull/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.