Spring cleaning fall 2017 #48
Conversation
| @@ -378,7 +378,6 @@ if (!module.parent) { | |||
| log.fatal({err}, 'Unhandled Promise Rejection!'); | |||
| throw err; | |||
| }); | |||
| require('source-map-support').install(); | |||
package.json
Outdated
| "eslint": "^4.9.0", | ||
| "eslint-config-taskcluster": "^2.0.0", | ||
| "eslint-plugin-taskcluster": "^1.0.2", | ||
| "eslint-config-taskcluster": "^3.0.0", |
There was a problem hiding this comment.
oh neat! we declared all the deps as deps of this package? great!
Procfile
Outdated
| @@ -1 +1 @@ | |||
| web: node lib/main start | ./node_modules/.bin/bunyan -o short | |||
There was a problem hiding this comment.
Sorry, I'd like to leave this as lib/. This tool was the first to use babel-less node again, and has used lib/ from day one. Best practises should have been written after examining prior art like this repository.
As for the reason, this code is not source for a compiler, rather it's executable code.
There was a problem hiding this comment.
I find these little differences trip me up repeatedly when working across multiple services, for example when upgrading a widely-used TC library. Remembering that ec2-manager is "different" is just a little cognitive drag.
It will take quite a bit of time to shift every other service over to lib/, rather than shifting this one to src/. Is it worth spending that time for three letters?
There was a problem hiding this comment.
We agreed to just leave these different.
src/main.js
Outdated
| @@ -82,6 +83,21 @@ let load = loader({ | |||
| }, | |||
| }, | |||
|
|
|||
| docs: { | |||
There was a problem hiding this comment.
do we have something here to ensure that docs are only published for those environments which have requested publishing in their config.yml entry?
There was a problem hiding this comment.
The default is
publish: process.env.NODE_ENV == 'production',
so this shouldn't need any special handling.
There was a problem hiding this comment.
nope, NODE_ENV is set to production even for staging apps. We should also not be reading the NODE_ENV variable outside of lib/main.js
There was a problem hiding this comment.
OK, then we should pass an explicit publish to this argument, based on a config.yml value.
package.json
Outdated
| @@ -19,7 +19,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "aws-sdk": "^2.172.0", | |||
| "bluebird": "^3.5.0", | |||
| "bluebird": "^3.5.1", | |||
There was a problem hiding this comment.
I'm pretty sure we're not actually using bluebird at all... It can probably be deleted.
| @@ -56,7 +55,7 @@ production: | |||
| env: production | |||
| forceSSL: true | |||
| trustProxy: true | |||
| baseUrl: 'https://ec2-manager.herokuapp.com/v1' | |||
There was a problem hiding this comment.
I was under the impression that the base url was supposed to have the /v1 in it
src/main.js
Outdated
| @@ -119,7 +119,7 @@ let load = loader({ | |||
| instancePubKey: cfg.app.instancePubKey, | |||
| state: state, | |||
| regions: cfg.app.regions, | |||
| apiBaseUrl: cfg.server.baseUrl, | |||
| apiBaseUrl: cfg.server.publicUrl + '/v1', | |||
There was a problem hiding this comment.
ahh, you're adding it here. Let's leave this as a hard coded value in the config.
There was a problem hiding this comment.
This is bringing the approach into line with taskcluster-queue and other services. That service doesn't send the API base URL as context, but does include it as an argument to v1.setup(..).
djmitche
force-pushed
the
spring-cleaning-fall-2017
branch
from
013daab
to
e40ba8a
Compare
|
I dropped the lib/ -> src/ move. I think that handling |
|
Conversation yesterday indicated @jhford is manually rebasing this and will land. |
|
I don't know if this ever landed, but it's too far bitrotted to recover now.. |
No description provided.