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

RESOLVED WONTFIX

Status

Taskcluster
General
RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: jhford, Unassigned)

Tracking

Details

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.
@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)
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Component: TaskCluster → General
Product: Testing → Taskcluster
Target Milestone: --- → mozilla41
Version: unspecified → Trunk
Resetting Version and Target Milestone that accidentally got changed...
Target Milestone: mozilla41 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.