1128931 - tc client libs should use the same env var for tc credentials as the base.config thing

Status: RESOLVED WONTFIX

(Taskcluster :: General, defect)

Description

[John Ford [:jhford] CET/CEST Berlin Time]

We should pick up env vars taskcluster_credentials_clientId and taskcluster_credentials_accessKey in client libraries to find credentials.  I know that we could create a client object programatically with the credentials, but if we pick the defaults out of the environment at all, we should do it in a useful way.

Comment 1

[Jonas Finnemann Jensen (:jonasfj)]

@jhford,
I assume this is because you don't want to have to load them in bin/server.js, etc...

We already default to environment variables:
  TASKCLUSTER_CLIENT_ID,
  TASKCLUSTER_ACCESS_TOKEN, and
  TASKCLUSTER_CERTIFICATE (optionally, if you're using temp credentials)

I chose not to use these for injection of credentials on heroku because in any code I write I always want the credentials to be explicitly loaded and provided.

Using env vars like this is good if you're writing a quick and dirty node script for the cli, or if you're prototyping something in node cli interface. But for robust applications, it's IMO preferable to have a declaration of which configuration keys exists (typically config/defaults.js) and a declaration of which environment variables may affect the application (typically bin/server.js, bin/<...>.js).

Like I said earlier we need to clean up the config story, use YAML or something... and simplify how environment variables are mixed in. But regardless of how we do it, I'm fairly confident we want config injection to be explicit, not accidental.
(global state is bad, we had a lot of it initially - let's just say it was hard to test)

Comment 2

[Jonas Finnemann Jensen (:jonasfj)]

Note, I think it's better to rethink the naming of env vars we load...

Perhaps, we should load configs in order like this:
 1. config/defaults.js
 2. config/production.js or config/test.js or config/localhost.js
 3. config/envs.js

Where envs.js is something like this:
module.exports = {
   taskcluster: {
     credentials: {
        clientId:          process.env.TASKCLUSTER_CLIENT_ID,
        accessToken:       process.env.TASKCLUSTER_ACCESS_TOKEN
     }
   }
}

And then we don't need to have any loading of environment variables in bin/server.js, etc.
This way environment variables can also be named whatever we like, instead of being tied to the JSON structure.
And if done in config/envs.js it would be rather explicit which env vars we load.

But this is a discussion for the config refactoring bug, see bug #1084661

Comment 3

[Pete Moore [:pmoore][:pete]]

Resetting Version and Target Milestone that accidentally got changed...