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

Rake restart task no longer loads entire Rails environment when run #19169

Merged
merged 1 commit into from Mar 2, 2015

Conversation

Projects
None yet
9 participants
@hjoo
Contributor

hjoo commented Mar 2, 2015

Fixes #18876. Rake restart touches tmp/restart.txt to restart
application on next request. Updated test and documentation accordingly.

@arthurnn

This comment has been minimized.

Member

arthurnn commented Mar 2, 2015

:shipit:

@eileencodes

This comment has been minimized.

Member

eileencodes commented Mar 2, 2015

Probably not going to affect it but I'm waiting til CI finishes to merge 😁

@kaspth

This comment has been minimized.

Member

kaspth commented Mar 2, 2015

@hjoo your commit message is off as you've already added the rake task. Can you update your commit message to say the task no longer loads the whole Rails environment when run 😄

If you can, a quick explanation of why the task doesn't need the environment would be great too 👍

Rake restart task no longer loads entire Rails environment when run.
The restart task does not need access to models or other classes and
helpers from the application environment.

@hjoo hjoo force-pushed the hjoo:rake_restart branch to beaf8cb Mar 2, 2015

@hjoo

This comment has been minimized.

Contributor

hjoo commented Mar 2, 2015

Oops, fixed it! Thanks for the feedback. 👍

eileencodes added a commit that referenced this pull request Mar 2, 2015

Merge pull request #19169 from hjoo/rake_restart
Rake restart task no longer loads entire Rails environment when run

@eileencodes eileencodes merged commit 230393a into rails:master Mar 2, 2015

@kaspth

This comment has been minimized.

Member

kaspth commented Mar 2, 2015

Well done 👍

@waynerobinson

This comment has been minimized.

waynerobinson commented Mar 23, 2015

Shouldn't this also be rails restart to go with the style of #18878?

@olivierlacan

This comment has been minimized.

Contributor

olivierlacan commented Mar 24, 2015

@waynerobinson Considering this comment the idea is to route commands to rake anyway so this command would have to be added to the supported hybrid rails/rake commands.

@waynerobinson

This comment has been minimized.

waynerobinson commented Mar 24, 2015

@olivierlacan no problem, just making sure there is some consistency here (not that I whole-heartedly approve of everything rails).

@emilsoman

This comment has been minimized.

Contributor

emilsoman commented Mar 26, 2015

@hjoo the PR title is misguiding - it looks like the rake task was added in a different PR ?

@eileencodes

This comment has been minimized.

Member

eileencodes commented Mar 26, 2015

@emilsoman it was but the commit has the correct title. I've edited it but I don't think it's a big deal.

@eileencodes eileencodes changed the title from Created rake restart task. to Rake restart task no longer loads entire Rails environment when run Mar 26, 2015

@seanarnold

This comment has been minimized.

seanarnold commented Mar 26, 2015

I agree with @waynerobinson . if we're adding this then add rails restart too. Especially since you start the server with rails server, it makes sense (to me at least) to have a rails restart command.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Mar 26, 2015

@seanarnold it will be added.

@seanarnold

This comment has been minimized.

seanarnold commented Mar 26, 2015

👍

@rails rails locked and limited conversation to collaborators Mar 27, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.