Arvados

• Used localhost:nnnn in API and workbench application.default.yml and used "arvados.local" / "workbench.local" in application.yml.example files. This is in sync with previous setup + the tests are also using localhost:nnnn and hence it seemed better to stick with this usage pattern.

• Removed port number configuration in common sections of the yaml files and added those in the development / test / production modes separately.

Reviewing eb8dc9d

• In the Rails YAML files:
  • I think omitting configuration options from the test sections of application.yml.example could help avoid misleading our users. The default values provided should always work for tests, and strange failures might occur if users change those. Hinting that they can be changed by including them in the example might confuse a few users.
  • And then sort of the mirror of that, I think it would be a little safer to omit URLs from the development section of application.default.yml. My main concern is avoiding situations where our code talks to other servers on the host (which might not be another Arvados server, especially since 3000 is the default Rails development port) before the user realizes what's going on. Suggesting the defaults in application.yml.example should be sufficient.
  • Unsetting variables with ~ is usually only needed when you're clearing a previous setting. It shouldn't be necessary for any of these settings in application.default.yml, since we're being careful not to define a value in the common section.
• A few more files need to be changed to match the new convention (I found these with git grep -E '\b30[0-3][0-3]\b'):
  • services/api/README
  • services/api/script/rails
  • services/keep/tools/traffic_test.py
• It's not a big deal, but just to make sure I understand, why does this branch change button text from "Stop" to "Pause" in apps/workbench/test/diagnostics/pipeline_test.rb?

Thanks.

Brett Smith wrote:

• Unsetting variables with ~ is usually only needed when you're clearing a previous setting. It shouldn't be necessary for any of these settings in application.default.yml, since we're being careful not to define a value in the common section.

IMO the ~ is worth including for clarity here, even when it doesn't change behavior.

If our recommendations change, or have changed (e.g., put the whole useful test setup in default.yml instead of expecting the test suite to copy yml.example to yml), we should update Hacking API server.

Brett: addressed your feedback.

• Regarding "In the Rails YAML files" :
  • I removed the config from test section in example yml
  • Updated the default yml files to use "~" instead. Please see Tom's comment regarding this.
• Regarding "A few more files need to be changed to match the new convention"
  • services/api/README : This file seems to be rails generic README with instructions as to how to generate a rails application. Hence, changing the port number 3000 did not seem correct and left it as is.
  • services/api/script/rails - I wonder how I lost my update to this! Anyways, updated this and the other file. Thanks for catching them.
• Regarding "apps/workbench/test/diagnostics/pipeline_test.rb": I retested this as part of my manual testing of port number updates and noticed that it is failing due to Peter's recent dashboard updates, which I had not noticed during testing the dashboard branch review.

f76e5864 is good to merge. Thanks.

Ward Vandewege wrote:

Reviewing 3991-docker-workbench-address:

• The change to docker/api/Dockerfile removes a warning. Cool.

Yeah, I thought it was failing there, so I fixed it, but it turned out to be a warning just before a different failure. Accidental improvement!

• The change to docker/passenger/Dockerfile seems fine. I note that it (greatly) increases the image build time because now Passenger needs to be rebuilt every time, but it's probably the right thing to do.

I'm not sure why this was a fatal problem on my setup but not others. I'm guessing it means some part of the (ruby/rvm/??) environment was leaking through from the host to the container (and in my case there was none on the host to leak) but this does seem more correct in any case.

• The change to docker/build_tools/build.rb which introduces

+ config['API_WORKBENCH_ADDRESS'] = 'http://localhost:9899'

is a problem, because this puts a run-time characteristic (the port the workbench docker container's web server is exposed at on the system running the container) hardcoded inside the image. That port is deliberately configurable in arvdock.
[...]
In other words, I think you should revert the changes to docker/api/application.yml.in, docker/build_tools/build.rb and docker/config.yml.example.

OK, I've changed the auto-configured setting to "false". If I remove it entirely, api server will rightly complain "you forgot to configure this, and you told me you're running in production mode". (And it seems like we might as well let people configure this via config.yml if they want to instead of forking application.yml.in, even though I suppose there are a hundred other things they can't configure this way...)

• services/api/app/controllers/static_controller.rb

When workbench_address is not set, this patch removes a helpful warning message and replaces it with a more mysterious 'Path not found'. I guess you've decided to just give up on the redirect feature without trying to help the admin to configure it. I suspect people will get hung up on this (rather than immediately understanding what is happening). Why not tell people something like "This is an Arvados API server. You probably want to visit the Workbench application for this API server." if you really don't want to point out the actual problem (i.e. workbench_address is unset)?

OK, I've changed it to "Oops, this is an API endpoint. You probably want to point your browser to an Arvados Workbench site instead."

(I don't like the "tell the admin to configure the thing" message. The config-checking code already forced the admin to make a choice about this feature during installation, and the admin decided to disable it. I want the application to respect that as a legitimate choice instead of suggesting to the users that the application isn't configured properly.)

Thanks. Update at 48d0472 + a805395

Excellent, LGTM. Thanks.