LD4P / sinopia_acl

node.js based code to interact with WebACL data on sinopia server
Apache License 2.0
0 stars 0 forks source link

use ENV vars when available, otherwise config values #66

Closed ndushay closed 5 years ago

ndushay commented 5 years ago

Using ENV vars when available will allow the populateEmptyTrellis script (a.k.a. bin/migrate) to be run against all the AWS deployed containers.

This approach accommodates the different trellis instances (local, dev, stage, prod) and the different AWS Cognito pools (development, staging, production). It is necessary prep work for #58, #59, and #60.

This PR accomplishes much of #63 but doesn't quite close it as these still remain:

- npm package will need to be updated

- possibly be more intentional about how sinopia_editor, sinopia_server, sinopia_indexing_pipeline get the correct code.
ndushay commented 5 years ago

Your comments make me inclined to get rid of the methods you so thoughtfully put in authenticateClient to get config values out of the constructor -- it seems just as clear, if not more so, for the constructor to set the vars directly:

  (in constructor)
    // Extracting these to methods makes for easier testing
    this.userPoolId = this.userPoolIdFromConfig()
...
  /**
   * @private
   */
  userPoolIdFromConfig() {
    config.get('userPoolId')
  }

vs

    this.userPoolId = config.get('userPoolId')

I'm inclined to go with the latter - any objection?

ndushay commented 5 years ago

Do you think this reference should be in config as well? process.env.COGNITO_ADMIN_PASSWORD ?

I don't want to encourage that being put in the code anywhere, but I could add a comment to that effect in the config ...

mjgiarlo commented 5 years ago

@ndushay :speech_balloon:

Your comments make me inclined to get rid of the methods you so thoughtfully put in authenticateClient to get config values out of the constructor -- it seems just as clear, if not more so, for the constructor to set the vars directly:

  (in constructor)
    // Extracting these to methods makes for easier testing
    this.userPoolId = this.userPoolIdFromConfig()
...
  /**
   * @private
   */
  userPoolIdFromConfig() {
    config.get('userPoolId')
  }

vs

    this.userPoolId = config.get('userPoolId')

I'm inclined to go with the latter - any objection?

:100: agree! We can/should use config.get() everywhere.

mjgiarlo commented 5 years ago

@ndushay :speech_balloon:

Do you think this reference should be in config as well? process.env.COGNITO_ADMIN_PASSWORD ?

I don't want to encourage that being put in the code anywhere, but I could add a comment to that effect in the config ...

Yeah, ideally all stuff injected via env would live in our config. It's OK if the default value is '' or null or undefined, I reckon.