heroku / buildpacks-ruby

Heroku's Cloud Native Buildpack for Ruby applications.
BSD 3-Clause "New" or "Revised" License
25 stars 5 forks source link

Hint missing values for `docker run` #306

Closed schneems closed 1 month ago

schneems commented 1 month ago
$ rails server
=> Booting Puma
=> Rails 7.1.3.2 application starting in development 
=> Run `bin/rails server --help` for more startup options
W, [2024-07-18T18:03:29.042235 #29235]  WARN -- : heinous_inline_partial initialized, found: {"app/views/comments/_threads.html.erb"=>"app/views/comments/_comment.html.erb", "app/views/home/index.html.erb"=>"app/views/stories/_listdetail.html.erb", "app/views/search/index.html.erb"=>"app/views/stories/_listdetail.html.erb"}
[29235] Puma starting in cluster mode...
[29235] * Puma version: 6.4.2 (ruby 3.3.1-p55) ("The Eagle of Durango")
[29235] *  Min threads: 4
[29235] *  Max threads: 4
[29235] *  Environment: development
[29235] *   Master PID: 29235
[29235] *      Workers: 3
[29235] *     Restarts: (✔) hot (✔) phased
[29235] * Listening on http://127.0.0.1:3000/
[29235] * Listening on http://[::1]:3000
[29235] Use Ctrl-C to stop
[29235] - Worker 0 (PID: 29264) booted in 0.03s, phase: 0
[29235] - Worker 2 (PID: 29266) booted in 0.03s, phase: 0
[29235] - Worker 1 (PID: 29265) booted in 0.03s, phase: 0
^C[29235] - Gracefully shutting down workers...
[29235] === puma shutdown: 2024-07-18 18:03:47 -0700 ===
[29235] - Goodbye!
Exiting

$ docker run --rm -p 8080:3000 sample-app
/layers/heroku_ruby/gems/ruby/3.3.0/gems/thor-1.3.0/lib/thor/parser/arguments.rb:143:in `parse_numeric': Expected numeric value for '--port'; got "" (Thor::MalformattedArgumentError)
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/thor-1.3.0/lib/thor/parser/options.rb:290:in `parse_peek'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/thor-1.3.0/lib/thor/parser/options.rb:116:in `parse'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/thor-1.3.0/lib/thor/base.rb:95:in `initialize'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/thor-1.3.0/lib/thor/invocation.rb:26:in `initialize'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/thor-1.3.0/lib/thor/shell.rb:45:in `initialize'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/railties-7.1.3.2/lib/rails/command/environment_argument.rb:17:in `initialize'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/railties-7.1.3.2/lib/rails/commands/server/server_command.rb:126:in `initialize'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/thor-1.3.0/lib/thor.rb:523:in `new'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/thor-1.3.0/lib/thor.rb:523:in `dispatch'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/railties-7.1.3.2/lib/rails/command/base.rb:73:in `perform'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/railties-7.1.3.2/lib/rails/command.rb:71:in `block in invoke'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/railties-7.1.3.2/lib/rails/command.rb:149:in `with_argv'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/railties-7.1.3.2/lib/rails/command.rb:69:in `invoke'
        from /layers/heroku_ruby/gems/ruby/3.3.0/gems/railties-7.1.3.2/lib/rails/commands.rb:18:in `<top (required)>'
        from bin/rails:4:in `require'
        from bin/rails:4:in `<main>'

This is failing because this is the command we're trying to run:

fn default_rails() -> Process {
    ProcessBuilder::new(process_type!("web"), ["bash"])
        .args([
            "-c",
            &[
                "bin/rails server",
                "--port \"$PORT\"",
                "--environment \"$RAILS_ENV\"",
            ]
            .join(" "),
        ])
        .default(true)
        .build()
}

It's looking for a $PORT but cannot find one. But the error doesn't say that.

Options

$ cat foo.sh
#!/usr/bin/env bash
set -eu

echo "$PORT"
⛄️ 3.3.1 🚀 /tmp/b4b31c92974b133cfc82b5e35659eb77
$ ./foo.sh
./foo.sh: line 4: PORT: unbound variable

Considerations

When this goes on Heroku we've talked about hiding the bash -c but it will be weird to see a command in the dashboard like bin/check_env_vars && rails server --port "$PORT". We could probably strip out set -eu the same way we strip out bash -c but it would be harder to strip out multiple invocations. Confusing too as to where the error is coming from.

edmorley commented 1 month ago

Use set -euo on the process type. I believe this will complain loudly that the variable doesn't exist i.e.

Using -u is one option, although the error message (a) might not be super clear to beginners, (b) can't be customised.

Another option would be to use the ${parameter:?word} bash shell parameter expansion feature: https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

For example:

$ bash -c 'echo "PORT=${PORT:?Error: PORT env var is not set!}"'
bash: line 1: PORT: Error: PORT env var is not set!

Also one option would be to set a default port instead of erroring, eg:

$ bash -c 'echo "PORT=${PORT:-5001}"'
PORT=5001
schneems commented 1 month ago

Also one option would be to set a default port instead of erroring, eg:

If we set that as a default it would look like this to the user:

$ docker run -it --rm -e PORT=5001 ruby-getting-started
[1] Puma starting in cluster mode...
[1] * Puma version: 6.4.2 (ruby 3.2.4-p170) ("The Eagle of Durango")
[1] *  Min threads: 5
[1] *  Max threads: 5
[1] *  Environment: production
[1] *   Master PID: 1
[1] *      Workers: 1
[1] *     Restarts: (✔) hot (✖) phased
[1] * Preloading application
[1] * Listening on http://0.0.0.0:5001
[1] Use Ctrl-C to stop
[1] ! WARNING: Detected running cluster mode with 1 worker.
[1] ! Running Puma in cluster mode with a single worker is often a misconfiguration.
[1] ! Consider running Puma in single-mode (workers = 0) in order to reduce memory overhead.
[1] ! Set the `silence_single_worker_warning` option to silence this warning message.
[1] - Worker 0 (PID: 14) booted in 0.0s, phase: 0

Then when they visit localhost:5001 they mysteriously see nothing because there's no bound port. At least if we force them to add the -e flag to the env var, it might hint to them that they also need to bind the port as well.

Wondering out loud: Does docker expose "here's the externally mapped ports" internally? Ideally we would be able to give good error/warning messages here. I.e. "you for got -e" and you forgot "-p`

Another option would be to use the ${parameter:?word} bash shell parameter expansion feature:

I like this option the best for the short term. In the long term, if we can get more info about docker from inside of docker then maybe we can have more sophisticated logic.

lilacstella commented 1 month ago

I believe Docker does not inform the user about what ports internally are exposed