adding a console to goliath #201

Merged
merged 6 commits into from Sep 8, 2012

6 participants

@nolman
postrank-labs member

A REPL!

    ruby examples/conf_test.rb -C -e test -c examples/config/conf_test.rb
    1.9.3p125 :002 > server.class
    => Goliath::Server < Object 
    1.9.3p125 :001 > server.config
    => {
        "global" => "my global",
        "test" => 0,
        "shared" => "shared"
    } 
@travisbot

This pull request passes (merged bfff33b into 0c0a871).

@igrigorik
postrank-labs member

Neat. But.. why? :-)

What does this give you over, let's say using pry, or similar debugger?

@nolman
postrank-labs member

I wanted a way to get a console in different environments, for example: checking/fixing data in prod, or playing around in development. IRB works fine when I am just working with plain old ruby objects, however when I need a redis connection (or other things that require a running reactor) it is nice to get them automatically loaded from my servers config opposed to having to set up a IRB session correctly, setting correct redis db etc

Could of course use redis-cli for some of it, however it is nice to have both a connection to your data store and the ruby that uses the data sometimes :)

Open to alternative approaches :)

@nolman
postrank-labs member

A bit more context, we have a continuous deployment server written with Goliath @Square (not really relevant to this pull request). Ended up having a few times where I have had to manually go into a irb session to fix some data or test something in development.

After doing it twice (and having to look at the config each time) I got lazy and ended up making a console script for our app.

I saw a old issue ( #32 ) and thought that it may be something others would benefit from however I could be the odd man out as no one has seemed to asked for it lately :)

@igrigorik
postrank-labs member

Right, ok.. that makes sense. I'm wondering if Pry could give you the same thing, and more, and with less overall code. Have you tried that route?

We recently had an issue around it, but I didn't track it down to the end: #197

@nolman
postrank-labs member

Good question, I am not familiar with Pry, looking into it now

@nolman
postrank-labs member

Hey Ilya,

Pry seems like a really excellent debugging tool, thanks for bringing it to my attention. I may be missing something however I am not sure how it can be used to load a Goliath app's config out of the box. Given that a config is eval'd in the context of a server. It seems like even with pry one would still need some boiler plate code to get a REPL that included a servers config in the context of a running reactor (I think the same code in run_console).

Sorry if I a missing something really basic.

Nolan

@igrigorik
postrank-labs member

Don't think you're missing anything.. Chances are, maybe I'm misunderstanding the intent.

If you start a server and just throw in a pry console in there... wouldn't that do the trick?

igrigorik { ~/Desktop } > ruby test.rb -sv
[57834:INFO] 2012-09-01 16:10:38 :: Starting server on 0.0.0.0:9000 in development mode. Watch out for stones.

From: test.rb @ line 6 API#response:

    6: def response(env)
 => 7:   binding.pry
    8:   [200,{},'hi']
    9: end

[1] pry(#<API>)> env
=> {"SERVER_SOFTWARE"=>"Goliath",
 "SERVER_NAME"=>"localhost",
 "rack.version"=>[1, 0],
 "rack.errors"=>#<IO:<STDERR>>,

My file:

require 'pry'
require 'goliath'

class API < Goliath::API

  def response(env)
    binding.pry
    [200,{},'hi']
  end
end
@nolman
postrank-labs member

Ya that would totally do the trick for development, in production what I would like to facilitate would be:

    ssh prod-server
    cd app-dir
    #invoke command to start a REPL
    #check/fix prod data

With the approach you have outlined we will need to check in a separate server file whose purpose is to just be a console. If you wanted to facilitate loading of the console with config we would need to pass in a separate arg for the config file. Currently we have a file checked in that does just that, starts a REPL with the config loaded. The goal of the pull request was to solve the problem in a more generic fashion so it would be reusable across Goliath projects (remove the one off file required in individual project(s)).

When I was opening the pull request I wasn't sure if a separate file to invoke (ie goliath-console or what have you) for the REPL or extending the current option parser would be a more appropriate approach. I can see how calling ruby server.rb -C can be surprising. Perhaps a separate file would be more appropriate?

In the end this pull request may represent a feature that is only useful for us (ie should not be merged).

@igrigorik
postrank-labs member

Gotcha, that use case makes sense.

@dj2 @mrflip @schmurfy @mikelewis @sleeper do you guys have any thoughts for, or against? I'm on the fence.

@dj2 dj2 commented on an outdated diff Sep 5, 2012
lib/goliath/runner.rb
@@ -2,6 +2,7 @@
require 'goliath/server'
require 'optparse'
require 'log4r'
+require 'irb'
@dj2
postrank-labs member
dj2 added a note Sep 5, 2012

Can this be done with autoload, or moved down into run_console so we don't pull in irb with every server?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mikelewis

I think that having a console to test code within an environment makes sense, however I don't think this should be within the server code.

My suggestion is that you factor out the common "bootstrap" code that can be used by both the server and console.

For example, you would just require 'goliath/bootstrap' and the server and console code can diverge at that point.

This way you don't sprinkle require 'irb' and the like in the server code.

@dj2 dj2 commented on an outdated diff Sep 5, 2012
lib/goliath/runner.rb
@@ -117,6 +118,7 @@ def initialize(argv, api)
@verbose = options.delete(:verbose)
@server_options = options
+ run_console if options[:console]
@dj2
postrank-labs member
dj2 added a note Sep 5, 2012

It's a little wierd that this happens in the initialize method. Would it be possible to do it down in run. Just a conditional at the top that does something like:

if options[:console]
run_console
return
end

Would also mean we can remove the Kernel.exit, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dj2 dj2 commented on an outdated diff Sep 5, 2012
lib/goliath/runner.rb
@@ -180,6 +184,20 @@ def load_plugins(plugins)
@plugins = plugins
end
+ # Runs a console in a running reactor
+ #
+ # @return [exit] This will exit the server after the REPL is terminated
+ def run_console
+ server = setup_server
+ EM.synchrony do
+ server.load_config
+ Object.send(:define_method, :server) { server }
@dj2
postrank-labs member
dj2 added a note Sep 5, 2012

We may want to bind this to something other then :server, Object#server seems a bit too generic, maybe Object#golitah_server?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dj2 dj2 commented on an outdated diff Sep 5, 2012
lib/goliath/runner.rb
@@ -169,6 +171,8 @@ def options_parser
opts.on('-v', '--verbose', "Enable verbose logging (default: #{@options[:verbose]})") { |v| @options[:verbose] = v }
opts.on('-h', '--help', 'Display help message') { show_options(opts) }
+ # Let the options complete parsing and the api get set before loading the console
@dj2
postrank-labs member
dj2 added a note Sep 5, 2012

This comment doens't make sense here as the next line doesn't start the REPL. Remove?

Is Common the right place for the option? Not really sure where it fits. If common is right, move -C up above -v.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dj2
postrank-labs member
dj2 commented Sep 5, 2012

The idea makes sense to me. Added a few code review comments. Would it make sense to make console something similar to application.rb? Loads up stuff, then passes the right bits to the runner (basiclaly what @mikelewis said).

@nolman
postrank-labs member

Thanks for the feedback @dj2 and @mikelewis I will look at splitting out 'goliath/bootstrap' later today.

Let me know if you want me to squash the commits back down into one.

@dj2
postrank-labs member
dj2 commented Sep 5, 2012

My preference is un-squashed, I like seeing the evolution of things, heh.

@nolman
postrank-labs member

Hey Gents, I just want to confirm my understanding of what this would look like.

As I understand it, if we wanted to make console look like application.rb we would need to pull the options parser (as it what determines if this is a console or a server) up out of runner and into the bootstrapper class so that bootstrap would call:

    run_target.run!(options, api_klass)
    # Where run_target is Goliath::Application or Goliath::Console

As this seems like a significant refactoring (happy to make it), I wanted to check back with you guys first to see what you thought and make sure that I was not misunderstanding something.

@dj2
postrank-labs member
dj2 commented Sep 7, 2012

Hm, I don't know if that will work. The one trick with the options parsing is that your server can add API options to the parser, that means you need access to the API class in order to create the options parser.

If we move that code to application.rb that means we'll break any custom server classes (using application.rb is optional) that exist as they'll lose they're options parser.

If the irb class is only loaded if you're using the console, I think that's sufficient to keep from bloating memory in normal operation?

@nolman
postrank-labs member

It would require the api class creation getting pulled out into bootstrap so that it could pass the option parser to the that instance and then pass that instance to Application.run!

Ya as it stands now irb will only be required when you load the console up and should have no impact on memory in normal operation. I think this should be good to go :)

@dj2 dj2 merged commit dfa8559 into postrank-labs:master Sep 8, 2012

1 check passed

Details default The Travis build passed
@esebastian

For quick reference, and doing a followup of the first commit with the REPL session example, the actual instance of the Goliath::Server seems to be named goliath_server instead of server

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