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

Reconsider removing Gemfile.lock from repo #425

Closed
hmistry opened this issue Mar 8, 2018 · 6 comments
Closed

Reconsider removing Gemfile.lock from repo #425

hmistry opened this issue Mar 8, 2018 · 6 comments

Comments

@hmistry
Copy link

hmistry commented Mar 8, 2018

This is in regards to the following commits: ff9a3dd and ff9a3dd

@aried3r Yehuda's recommendations to not check-in the lock file made sense at the time when locking dependencies was not a big practice, and it made sense to place the burden of having the CI's test the gem with latest dependencies. However it had an unforeseen side effect where new gem contributors had to deal with broken installs, thus discouraging them from contributing.

The Bundler team have reconsidered this recently and have changed the new gem templates to include Gemfile.lock in the repo. Given the state of things today, they felt it to be more important to have new contributors contributing and reduce hurdles for them. This can be achieved without trading off CI checks against latest dependencies because you can run the test suite twice, once with the lock file and second without the lock file. See this article: http://bundler.io/blog/2018/01/17/making-gem-development-a-little-better.html

Let's make it easier for new contributors to get a working setup quickly.

@aried3r
Copy link
Member

aried3r commented Mar 8, 2018

Great! I kinda thought the article was outdated but didn't stumble across your article back when I did the commit. I'll definitely re-add the Gemfile.lock. (Also grateful for PRs, haha)

I have two questions if that's ok. Is there anything we can do looking forward to Bundler 2.0 with gems.rb? Also, is the discussion David Rodriguez brought up available anywhere? I'd love to read up on that, your blog entry only mentions it, not sure if it happened on Discord, Slack, Github, real life.

@hmistry
Copy link
Author

hmistry commented Mar 9, 2018

Regarding gem.rb, it seems bundler 2.0 will support it but Gemfile will still remain.

There was some discussions in Slack but I got more info talking to him and Andre directly. This gist of it is in the comment above. It's always nice to learn the context.

@aried3r
Copy link
Member

aried3r commented Apr 11, 2018

Hey, I'm planning to do this this week. Seeing as you are the author of the article, what is your opinion regarding CI? Using option 1 (rm Gemfile.lock) or option 2 (bot)?

@hmistry
Copy link
Author

hmistry commented Apr 11, 2018

Option 2 with Dependabot is a really nice because it'll generate a PR like this example and you simply merge it. The downside is other PR's are not tested against latest dependencies.

I personally prefer option 1 but instead of removing the Gemfile.lock, have 2 Gemfiles - one with a lock and the other without. The trade-off with this is travis will run the tests twice but from a dev perspective, it's a lot clearer to see that tests passing with locked versions and tests not passing with latest versions of dependencies in each PR. See this example repo and Travis can be configured like:

gemfile:
  - Gemfile
  - Gemfile.latest

@aried3r
Copy link
Member

aried3r commented Apr 16, 2018

I re-added the Gemfile.lock file but haven't implemented option 1 or 2 to test the latest versions yet. The bot seems nice, but I'd don't spend enough time right now on rpush so I'd rather not have the constant PRs right now. Option 1 seems nice!

If I might ask a few more questions, should the packed .gem file contain the README.md? I saw paper_trail only ships very few files and directories, and I kinda like that. Currently we also ship the specs in Rpush.

@hmistry
Copy link
Author

hmistry commented Apr 18, 2018

There's no set convention. People in Bundler team don't ship specs with the gem. With Readme's it's a toss up - it depends on what percentage of your users open the installed the gem directory to read documentation and code vs going to the github repo.

mariannegru added a commit to mariannegru/rpush that referenced this issue May 20, 2018
* upstream/master:
  Add some info about the auth_key
  Update CHANGELOG.md
  Update CHANGELOG.md
  Re-add Gemfile.lock to repo. Fixes rpush#425
  Prepare 3.1.1 release
  Use latest gem release on Travis
  Add changelog entry [ci skip]
  Changed pluck(:id) to .ids
  rpush_notifications active_record performance improvements
  Prepare 3.1.0 release
  Update CHANGELOG.md [ci skip]
  Add support for Pushy service

# Conflicts:
#	lib/generators/rpush_migration_generator.rb
#	lib/rpush/client/redis/app.rb
#	spec/support/active_record_setup.rb
Adrian1707 pushed a commit to Adrian1707/rpush that referenced this issue Apr 24, 2024
Adrian1707 pushed a commit to Adrian1707/rpush that referenced this issue Apr 24, 2024
* upstream/master:
  Add some info about the auth_key
  Update CHANGELOG.md
  Update CHANGELOG.md
  Re-add Gemfile.lock to repo. Fixes rpush#425
  Prepare 3.1.1 release
  Use latest gem release on Travis
  Add changelog entry [ci skip]
  Changed pluck(:id) to .ids
  rpush_notifications active_record performance improvements
  Prepare 3.1.0 release
  Update CHANGELOG.md [ci skip]
  Add support for Pushy service

# Conflicts:
#	lib/generators/rpush_migration_generator.rb
#	lib/rpush/client/redis/app.rb
#	spec/support/active_record_setup.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants