Conversation
|
@djmitche please consider documenting them in the taskcluster-skeleton repository... I already started on the folder layout.... |
jhford
left a comment
jhford
left a comment
There was a problem hiding this comment.
Looks good to me. I didn't get a chance to run the tests, but I assume you did that and they pass. I hope you knew about the --fix flag to eslint for fixing all the lint :)
r+, with the fixes to the babel stuff.
| @@ -1,2 +1,2 @@ | |||
| web: babel-node bin/main.js server | |||
| scheduler: babel-node bin/main.js scheduler No newline at end of file | |||
| web: babel-node src/main.js server | |||
There was a problem hiding this comment.
so this might work, but it will end up actually not using the compiled (and tested) javascript code. Instead, what you should be doing is node lib/main.js ... for both entries.
There was a problem hiding this comment.
Oh, and also, this behaviour is depending on babel-node being installed, but that binary was removed (iirc) in an update to the 'babel' package a while ago, so this would likely fail on deploy as the required binary would be absent.
There was a problem hiding this comment.
Oh, good catch, thanks! It definitely won't work as-is!
| region: us-west-2 | ||
| apiVersion: 2014-01-01 | ||
| monitor: | ||
| mock: true |
There was a problem hiding this comment.
in other main.js files, we usually compare the environment to production and use that truthiness value instead of coding a true or false into the config files. not too important.
There was a problem hiding this comment.
It seems to go both ways (I copied this from another service). I'll add this to my list for best practices.
| setup: ({process, profile, cfg}) => monitor({ | ||
| project: 'taskcluster-hooks', | ||
| credentials: cfg.taskcluster.credentials, | ||
| mock: cfg.monitor.mock, // true in production, false in testing |
There was a problem hiding this comment.
this is where i'd do the mock: profile !== 'production' check.
| # USAGE: Run this file using `npm test` from repository root | ||
|
|
||
| mocha test/*_test.js | ||
| mocha .test/*_test.js |
There was a problem hiding this comment.
We can safely delete this file i believe? Its only purpose was to wrap the package.json script entry iirc, so now that you're calling into mocha directly, It's not important to exist.
There was a problem hiding this comment.
Another good catch :)
| "aws-sdk": "^2.2.0", | ||
| "azure-entities": "^0.9.0", | ||
| "azure-entities": "^0.9.1", | ||
| "babel": "^5.8.21", |
There was a problem hiding this comment.
This should probably be removed because it's no longer needed as babel-compile pulls in the correct version of babel-core.
|
@jhford I think that addresses the fixes you mentioned. I'm away until Monday, so there's plenty of time if you want to have another look -- otherwise I can just merge on Monday given the r+ above. |
|
Looks good to me! |
This was missed in conflict resolution in taskcluster#36.
3 participants
This adheres to modern standards for TC microservices.
I'm starting to write those standards down, too and will create a docs page with the results.
r? jhford since you built a lot of these modernizations :)