-
Notifications
You must be signed in to change notification settings - Fork 21.8k
Implement rake proxy for rails cli #22288
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
Implement rake proxy for rails cli #22288
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
@dharamgollapudi Did you happen to notice all the discussion in #21254 ? I was looking at doing something for #18878 also but got overwhelmed by the discussion there. |
@timbreitkreutz yes I did. My understanding per #18878 as well as follow up comments by @dhh and others in #21012, is that we needed a thin layer that proxies rake tasks, as we are not going to deprecate or move over all the existing tasks as they have dependencies. That is exactly what this pull request addresses, which handles all the rake tasks through rails cli. Also the implementation in #21254 probably may have some performance penalties as it seems to define methods that call the underlying rake tasks. As rails cli is not a service, this will happen on every run. |
This isn't exactly what we're going for. In addition to funneling tasks through Rails, we want to deprecate running tasks with Rake (regardless of whether we use Rake to run tasks internally). What we've been trying to do in #21254 is create a lightweight command structure to give us easier parameter parsing in the future. It's been taking longer than I've wanted, so I'm open to also move things along here. Thanks 👍 |
@dharamgollapudi hey, since writing the above message I've changed my opinion and think your change is good. Let me know if you still want to work on this and I'll review it more in depth. Thanks! ❤️ |
Sure, I can understand. We are generally going in that direction, but I don't think we will be there in time for Rails 5*. As such I think this is a great cover that gives leverage to work on the underpinnings without a deadline. I'll go through the PR now, 👍.
|
@@ -8,6 +8,7 @@ | |||
"db" => "dbconsole", | |||
"r" => "runner", | |||
"t" => "test", | |||
"k" => "rake" |
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.
Don't think querying rake version and help info is a common enough command to warrant an alias. Let's take it out.
This reverts commit 8c79db3.
668a66a
to
435e7f2
Compare
@kaspth Implemented the changes per the feedback. |
By running any of the following commands within the newly generated rails app: following is the output:
|
throwing an error message. If the command is valid, a method of the same name | ||
is called. | ||
If the command is part of the COMMAND_WHITELIST, a method of the same name is called, | ||
if not we proxy it to rake. |
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'd rather keep the old comment, we already copy the implementation, so we don't need to add more implementation details.
It seems we should just make the rake tasks flow straight with:
So a one space delimiter and then 3 spaces minimum. Note: this means we have to make the |
Rake.application.load_rakefile | ||
Rake.application.top_level | ||
end | ||
end |
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.
Should be:
def run_rake_task(task)
ARGV.unshift(task) # Prepend the command, so Rake knows how to run it.
Rake.application...
end
@dharamgollapudi thanks so much for the work! We needed this in sooner, so I addressed my comment in the merge commit here: 3e65c3d. Thanks again ❤️ |
rails stats
,rails db:version
, etc)rake
subcommand with shortcut set tok
which can be used to queryversion and help info of rake, which supports all three formats as
-h
,--help
andhelp
.rake
tasks throughrails
#21012