Arvados

Ward Vandewege wrote:

Notes:

1. By completely skipping the reading of the config file when ARVADOS_API_HOST and ARVADOS_API_TOKEN are set in the environment, you're creating a potentially confusing situation, especially if we add future settings to that configuration file.

Added a comment. (Currently all other settings are ignored anyway.)

I think what you want to do here is not allowing the overriding of ARVADOS_API_HOST_INSECURE in the config file if ARVADOS_API_HOST is set via the environment. But everything else should still apply. And it should not matter if ARVADOS_API_TOKEN is set via the environment or via the config file or not at all.

Also wanted to avoid uselessly reading the file, since it can have no effect at this point other than spitting errors (e.g., file is unreadable, HOME is not set).

Looking at this again I noticed a "catch all exceptions, assume/report that $HOME is unset, and continue running" pattern. Kept the logic, but reported the actual error instead of the expected error.

(Also fixed NoMethodError: undefined method `debuglog' for #<Arvados:0x007f8861dd8b00>)

2. this isn't quite perfect yet:

- if var and val
+ if val

Apparently it is possible to assign values to a hash key that is empty. So with this in the config file

=value

that is what would happen. I suggest

if not var.empty? and val

instead.

Fixed.

Tom Clegg wrote:

Added a comment. (Currently all other settings are ignored anyway.)

I think what you want to do here is not allowing the overriding of ARVADOS_API_HOST_INSECURE in the config file if ARVADOS_API_HOST is set via the environment. But everything else should still apply. And it should not matter if ARVADOS_API_TOKEN is set via the environment or via the config file or not at all.

Also wanted to avoid uselessly reading the file, since it can have no effect at this point other than spitting errors (e.g., file is unreadable, HOME is not set).

As written, yeah.

Looking at this again I noticed a "catch all exceptions, assume/report that $HOME is unset, and continue running" pattern. Kept the logic, but reported the actual error instead of the expected error.

(Also fixed NoMethodError: undefined method `debuglog' for #<Arvados:0x007f8861dd8b00>)

Cool.

2. this isn't quite perfect yet:

- if var and val
+ if val

Apparently it is possible to assign values to a hash key that is empty. So with this in the config file

=value

that is what would happen. I suggest

if not var.empty? and val

instead.

Fixed.

Thanks, ready to merge.