Arvados

Changes at arvados|cdd113a87466d525372884b5539df404c624d1c1 branch 23296-wb-invert-config-logic
Tests developer-run-tests-services-workbench2: #1671

Note: Another fun thing I realized we can do is not have a config file at all and just set REACT_APP_ARVADOS_API_HOST in your shell RC, that was always possible and avoided the conflict, but this change adds the flexibility of using both with an easier time overriding the ENV config where needed whereas the config file is harder to override if it takes precedence.

• All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
  • I split out the WB config file loading into a separate function for better readability and consistent error handling
  • Both the ENV and file config functions now return promises and throw errors if they fail to load config (as opposed to previously the ENV would default to undefined API_HOST, which doesn't seem useful)
  • The config precedence is now inverted so that it tries to get the config from the ENV var first, before trying the config file
  • In both cases, it warns the console if it failed to load either, but throws an error if the config file isn't found, similar to how it throws when config is found but API_HOST is not set
  • Since errors thrown in the main fetchConfig were not caught by the consumer in index.tsx, I added a catch there to log it and specify that it's a fatal error.
  • Now that ENV var takes precedence, the check for config file is no longer needed in run-integration-tests
  • I also found a stray REACT_APP_ARVADOS_API_HOST in the .env, so I removed that since it's set to a cluster that doesn't seem to exist anymore, and having it set there only adds a hurdle to using the config file since it would take precedence.
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • none
• Code is tested and passing, both automated and manual, what manual testing was done is described.
  • Tested running yarn dev server and integration tests at the same time
• The tested code incorporates recent main branch changes.
  • recent
• New or changed UI/UX has gotten feedback from stakeholders.
  • none
• Documentation has been updated.
  • n/a
• Behaves appropriately at the intended scale (describe intended scale).
  • no change
• Considered backwards and forwards compatibility issues between client and server.
  • none
• Follows our coding standards and GUI style guidelines.
  • yes

One minor nit: In testing, I tried to make my REACT_APP_ARVADOS_API_HOST "tordo.arvadosapi.org" (not *.com) and the only error I received was Fatal error: Network Error which is pretty vague. I think just as a safeguard, checking REACT_APP_ARVADOS_API_HOST against ^[A-Za-z0-9]{5}\.arvadosapi\.com$ and throwing an error if it's wrong might be helpful.

ETA: Not that exact regex, one that matches the URL the customer uses.

Lisa Knox wrote in #note-8:

One minor nit: In testing, I tried to make my REACT_APP_ARVADOS_API_HOST "tordo.arvadosapi.org" (not *.com) and the only error I received was Fatal error: Network Error which is pretty vague. I think just as a safeguard, checking REACT_APP_ARVADOS_API_HOST against ^[A-Za-z0-9]{5}\.arvadosapi\.com$ and throwing an error if it's wrong might be helpful.

ETA: Not that exact regex, one that matches the URL the customer uses.

Unfortunately there is no such regexp. An API host could be at any host name and I would prefer we don't constrain that.

I understand the concern about the error being vague. If it's possible to include more details in it, I'd prefer to go that route. I don't know if that's even our code though.

arvados|25b8ba6148035a3d8d7642cf26d0e1dbf3f63b2a

I added a catch-and-rethrow to add the API_HOST url to any error stemming from trying to request the cluster config to make any network errors clearer, it now looks like this:

Fatal error: Failed to fetch cluster config from http://example.com: Network Error

developer-run-tests-services-workbench2: #1672

LGTM!