-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove Foreman gem #5821
Remove Foreman gem #5821
Conversation
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.
Seems very reasonable, though the PR that introduced this is from March 2019, which makes me think there was something else going on
I couldn't find it though
@rhymes Nothing was going on from what I can tell. Thor was updated, Foreman wasn't yet. We left a note to update Foreman once it had released the new version with an updated dependency, which didn't happen until 0.86.0 released in October 2019. Foreman 0.85.0 had the dependency listed as |
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 still think we shouldn't have foreman in our Gemfile
:
Ruby users should take care not to install foreman in their project's Gemfile. See this wiki article for more details.
and the wiki link is title "Don't Bundle Foreman" :D https://github.com/ddollar/foreman/wiki/Don't-Bundle-Foreman
though having it enabled only in development mode should be safe enough
@rhymes Agreed, I usually don't bundle it. But since this project does and had a pretty outdated comment, I updated Foreman. I'd be just as happy removing it. |
Let's see what @maestromac thinks, I'm definitely down to removing it and add it to the installation guide |
Oh yes! Let's remove |
@rhymes & @maestromac Removed Foreman and updated the docs. |
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.
Loks good!
I wonder if it'd make sense to link this new "others" page in a note in this other page: https://docs.dev.to/getting-started/start-app/
In case people run bin/startup
and they don't have foreman installed:
➜ devto git:(citizen428/update-foreman) ✗ bin/startup
== STARTING UP ==
Traceback (most recent call last):
4: from /Users/rhymes/.rvm/gems/ruby-2.6.5/bin/ruby_executable_hooks:24:in `<main>'
3: from /Users/rhymes/.rvm/gems/ruby-2.6.5/bin/ruby_executable_hooks:24:in `eval'
2: from /Users/rhymes/.rvm/gems/ruby-2.6.5/bin/foreman:23:in `<main>'
1: from /Users/rhymes/.rvm/gems/ruby-2.6.5/gems/bundler-2.0.2/lib/bundler/rubygems_integration.rb:480:in `block in replace_bin_path'
/Users/rhymes/.rvm/gems/ruby-2.6.5/gems/bundler-2.0.2/lib/bundler/rubygems_integration.rb:460:in `block in replace_bin_path': can't find executable foreman for gem foreman. foreman is not currently included in the bundle, perhaps you meant to add it to your Gemfile? (Gem::Exception)
== Command ["bundle exec foreman start -f Procfile.dev"] failed ==
@rhymes Makes total sense, added a note there. |
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.
LGTM!
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 this makes sense 👍
What type of PR is this? (check all applicable)
Description
This removes the Foreman gem, as per the author's recommendation:
Related Tickets & Documents
n/a
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?