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

support - as an argument to rails runner #26343

Merged
merged 1 commit into from Jul 17, 2017
Merged

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Aug 31, 2016

Summary

in Rails 4.0, you could use /dev/stdin on both Linux and Mac, but with
the switch to Kernel.load in Rails 4.1, this broke on Linux (you get
a LoadError). Instead, explicitly detect - as meaning stdin, then
read from stdin explicitly, instead of performing file gymnastics. This
should now work on any platform uniformly.

Other Information

Passing a script via stdin is useful when you're sshing to a server,
and the script you want to run is stored locally. You could theoretically
pass the entire script on the command line, but in reality you'll run
into problems with the command being too long.

@rails-bot
Copy link

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

@sgrif
Copy link
Contributor

sgrif commented Aug 31, 2016

Would it make sense for us to just always read from stdin when stdin isn't a TTY? /cc @rafaelfranca

@ccutrer
Copy link
Contributor Author

ccutrer commented Aug 31, 2016

@sgrif: I'd say no. I would assume it would be a normal use case to do something like:

cat some_data.csv | rails runner my_processing_script.rb

@sgrif
Copy link
Contributor

sgrif commented Aug 31, 2016

I'm actually wondering if this is just a bug in Ruby that should be handled upstream. It strikes me as odd that Kernel.load("/dev/stdin") would work on mac but not linux.

@ccutrer
Copy link
Contributor Author

ccutrer commented Aug 31, 2016

Yeah, I thought it was odd. But with this, it's also portable to Windows

@sgrif
Copy link
Contributor

sgrif commented Aug 31, 2016

I think it's reasonable. Can you fix the code climate issues. Can you also give some example of another application that uses - to specify "read from stdin"?

@ccutrer
Copy link
Contributor Author

ccutrer commented Sep 1, 2016

commit updated; it should pass code climate this time, and I think that's what you meant by an improved example

@jbbarth
Copy link
Contributor

jbbarth commented May 17, 2017

Another useful case is running rails scripts inside a docker container: copying the file inside the container is a gymnastic you probably want to avoid, VS cat script.rb | docker exec -it <container_id> rails runner -.

@sgrif @ccutrer it would be cool to have this included in a future version of rails. Can I (or others) help to make this happen?

Copy link
Contributor

@sgrif sgrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rebase this?

@@ -30,6 +30,9 @@
opts.separator ""
opts.separator " rails runner path/to/filename.rb"
opts.separator " This runs the Ruby file located at `path/to/filename.rb` after loading the app"
opts.separator ""
opts.separator " cat local_script.rb | ssh myserver '/var/www/myapp/bin/rails runner -'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This buries the example quite a bit. How about just rails runner -?

in Rails 4.0, you could use `/dev/stdin` on both Linux and Mac, but with
the switch to Kernel.load in Rails 4.1, this broke on Linux (you get
a LoadError). Instead, explicitly detect `-` as meaning stdin, then
read from stdin explicitly, instead of performing file gymnastics. This
should now work on any platform uniformly.

Passing a script via stdin is useful when you're sshing to a server,
and the script you want to run is stored locally. You could theoretically
pass the entire script on the command line, but in reality you'll run
into problems with the command being too long.
@ccutrer
Copy link
Contributor Author

ccutrer commented Jul 17, 2017

Rebased with conflicts resolved, and simplified the usage example.

@sgrif sgrif merged commit bfea6f2 into rails:master Jul 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants