Split out fxa-basket-proxy into a standalone project. #2

rfk · 2015-09-10T03:09:30Z

This commit imports the fxa-basket-proxy code from [1] as a standalone
project, splitting it up into smaller chunks and adding some basic
repo infrastructure and tests. The logic of the code itself is unchanged
apart from some config-loading and logging tweaks.

[1] https://github.com/mozilla/fxa-content-server/blob/13a59b2457d373190f1853c513604a5b72bb5106/server/bin/basket-proxy-server.js

rfk · 2015-09-10T03:10:28Z

I'm opening this for anyone who'd like to give it an initial look-over to offer feedback, but will continue to work on it a little until I'm happy with the over all shape. @shane-tomlinson and/or @jrgm any thoughts on the proposed split-out from content-server repo?

jrgm · 2015-09-10T04:40:15Z

Looks good. I have one question: is the intent to run this on the same instances as fxa-content-server, or as a separate service? I'd prefer the former.

rfk · 2015-09-10T04:51:08Z

is the intent to run this on the same instances as fxa-content-server, or as a separate service?

Your call :-)

Running on the same instances sounds fine to me. In that case, would it make sense for it to continue sharing the same .json config file as the main content-server process? (I changed a few things in the config-file handling to be more consistent with the way other backend services to them, but can easily change back for compatibility).

I think we should also move the basket SQS consumer from [1] into this repo, to keep all the basket stuff together. But that just pulls things off a queue and posts them to an endpoint, so ISTM there should be no problem moving that to the content-server instances as well if necessary.

[1] https://github.com/mozilla/fxa-auth-server/blob/master/bin/basket.js

pdehaan · 2015-09-10T17:06:22Z

CONTRIBUTING.md

+- Your patch should include new tests that cover your changes. It is your and your reviewer's responsibility to ensure your patch includes adequate tests.
+
+When submitting a PR:
+- You agree to license your code under the project's open source license ([MPL 2.0](/LICENSE)).


I think we need to add a /LICENSE file.

pdehaan · 2015-09-10T17:10:34Z

grunttasks/bump.js

+module.exports = function (grunt) {
+  grunt.config('bump', {
+    options: {
+      files: [ 'package.json', 'npm-shrinkwrap.json' ],


Not sure if we have a shrinkwrap file in this repo [yet].

It looks like @rfk has added it.

pdehaan · 2015-09-10T17:12:52Z

grunttasks/jscs.js

+        '<%= eslint.files %>'
+      ],
+      options: {
+        config: '.jscsrc',


We may need to copy over the .jscsrc file as well. ¯_(ツ)_/¯

@rfk - @pdehaan is correct, there is no .jscsrc file in the repo.

Ugh, it was being ignored by the .gitignore so didn't get checked in, I've added it now

pdehaan · 2015-09-10T17:20:40Z

lib/basket/index.js

+  return {
+    status: 'error',
+    desc: String(desc),
+    code: code || module.exports.UNKNOWN_ERROR


module.exports.errors.UNKNOWN_ERROR?

pdehaan · 2015-09-10T18:07:19Z

.eslintignore

+.tmp/**
+app/bower_components/**
+app/scripts/vendor/**
+dist/**


I don't know if any of these paths apply to this repo. We may be able to nuke this file.

pdehaan · 2015-09-10T18:09:57Z

README.md

+
+| TASK | DESCRIPTION |
+|------|-------------|
+| `grunt lint` | run ESLint, JSONLint, and JSCS (code style checker) on the code. |


Technically I don't think we're using JSONLint anywhere here.

pdehaan · 2015-09-10T18:10:40Z

CONTRIBUTING.md

+
+Before submitting a PR:
+- Your code must run and pass all the automated tests before you submit your PR for review. "Work in progress" pull requests are allowed to be submitted, but should be clearly labeled as such and should not be merged until all tests pass and the code has been reviewed.
+  - Run `grunt eslint` to make sure your code passes linting.


We should probably generic this down to grunt lint (which runs ESLint and JSCS; https://github.com/mozilla/fxa-basket-proxy/pull/2/files#diff-04c6e90faac2675aa89e2176d2eec7d8R25)

pdehaan · 2015-09-10T18:15:09Z

README.md

@@ -0,0 +1,33 @@
+# Firefox Accounts Basket Proxy
+
+[![Build Status: Travis](https://travis-ci.org/mozilla/fxa-basket-proxy.svg?branch=master)](https://travis-ci.org/mozilla/fxa-basket-proxy)


I think I enabled Travis-CI for this repo, but not sure how to set up coveralls below.

pdehaan · 2015-09-10T18:19:00Z

grunttasks/lint.js

+  grunt.registerTask('lint', 'lint all the things', SUBTASKS);
+  grunt.registerTask('quicklint', 'lint the modified files', SUBTASKS.map(function (task) {
+    return 'newer:' + task;
+  }));


If we don't plan on using the Git pre-commit hooks in this repo, we can remove the quicklint task and probably remove a dependency or two.

pdehaan · 2015-09-10T18:23:36Z

package.json

+    "grunt-nodemon": "^0.4.0",
+    "grunt-nsp-shrinkwrap": "0.0.3",
+    "grunt-todo": "0.5.0",
+    "husky": "0.9.2",


If we don't plan on adding git pre-commit hooks to this repo (see #2 (comment)), we can remove the husky and grunt-newer dependencies above.

pdehaan · 2015-09-10T20:44:55Z

package.json

+    "mocha-text-cov": "^0.1.0",
+    "nock": "2.10",
+    "proxyquire": "1.6.0",
+    "sinon": "1.15.4",


I don't think we're using proxyquire or sinon anywhere in these files.

rfk · 2015-09-10T21:13:48Z

@pdehaan ❤️

pdehaan · 2015-09-10T21:51:01Z

@rfk: To be clear, I add nothing to the conversation. I'm happy to merge this as-is and we can do future nit PRs to smooth the edges.

rfk · 2015-09-10T23:01:47Z

No no, I will definitely go through and apply these fixes. It's the sort of stuff that I can no longer notice because I've been shuffling all these files around for too long, and I deliberately pushed it thinking "I bet @pdehaan will notice a bunch of nits in the repo infrastructure" :-)

rfk · 2015-09-11T04:57:20Z

(force-pushed with @pdehaan nits addressed, and travis does indeed appear to be running it)

rfk · 2015-09-11T07:51:10Z

OK, test coverage up to 95%, I think this is ready for a preliminary r?. @zaach since Shane's PTO this weekend, would you mind giving this a look over?

Concurrently, I'll work on getting it running locally in fxa-local-dev to check that functional tests still pass.

pdehaan · 2015-09-11T22:17:41Z

Gruntfile.js

+
+module.exports = function (grunt) {
+  // load all grunt tasks based on environment
+  if (process.env.NODE_ENV && process.env.NODE_ENV === 'production') {


Nit: this may need some tweaking as well.
According to lib/config.js, it looks like the NODE_ENV may be "prod", not "production":

var conf = module.exports = convict({ env: { doc: 'The current node.js environment', default: 'prod', format: [ 'dev', 'test', 'stage', 'prod' ], env: 'NODE_ENV' },

Not sure if this is worth wrapping in a conf.get('env') === 'prod'; style thing, or maybe we need a better abstraction for determining if we're running in dev/stage/prod environment.

@rfk, @pdehaan - this is the same as what is used in the fxa-content-server. Should both be updated to use prod, or should prod be production? The inconsistency makes my head hurt.

I changed lib/config.js to match what's in content-server, so they can share the same config file in deployment.

shane-tomlinson · 2015-09-16T13:06:55Z

config/production.json

+  "env": "prod",
+  "basket": {
+    "apiUrl": "https://basket.mozilla.org/news"
+  }


Should log.format be changed to heka for production?

shane-tomlinson · 2015-09-16T13:07:25Z

config/local.json-dist

+  "publicUrl": "http://127.0.0.1:1114",
+  "env": "dev",
+  "log": {
+    "format": "pretty",


The default is pretty, so this shouldn't be needed.

shane-tomlinson · 2015-09-16T13:07:53Z

config/local.json-dist

@@ -0,0 +1,8 @@
+{
+  "publicUrl": "http://127.0.0.1:1114",


This is the same as the default value, is it needed?

shane-tomlinson · 2015-09-16T13:08:57Z

lib/routes/index.js

+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+module.exports = {
+  version: require('./version'),


Can you alphabetize these?

shane-tomlinson · 2015-09-16T13:09:15Z

lib/logging/summary.js

+        code: res.statusCode,
+        method: req.method,
+        path: req.path,
+        agent: req.headers['user-agent'],


Alphabetization would be nice here too.

shane-tomlinson · 2015-09-16T13:11:01Z

lib/routes/version.js

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/**


We use the core version fetch logic in a couple of repos, think we should extract it into its own library to share?

rfk · 2015-09-19T11:12:33Z

Thanks @shane-tomlinson! I've addressed all of your notes, except for the query about CORS * origin and #4 as a follow-up issue. Still to do:

make config file format compatible with fxa-content-server
integration test with fxa-local-dev

rfk · 2015-09-19T12:05:30Z

This seems to be working OK with mozilla/fxa-local-dev#32, I think it's ready for a final r?

shane-tomlinson · 2015-09-22T14:55:52Z

.gitignore

+!.git*
+!.eslint*
+!.npmignore
+!.travis.yml


We would want .gitignore, .eslint, and .travis.yml updates to be noticed, wouldn't we?

Yes, so IIUC this is saying "ignore all dot-files except the ones explicitly named here"

shane-tomlinson · 2015-09-22T14:57:44Z

grunttasks/eslint.js

+      eslintrc: '.eslintrc'
+    },
+    files: [
+      '{,bin/,config/,grunttasks/,lib/**/,test/,test/lib/mocks}*.js'


Woah, I've never seen this form.

Although, I am mildly curious about that "test/" trickery there and wonder why the glob isn't just "test/**/" (instead of "test/,test/lib/mocks").

test/lib/blanket.js is not our code so I excluded it from the checks, particularly to prevent being nagged about a missing MPL license header. I wonder if this fantastical glob syntax has a way to say "not this file" which would make it clearer...

You could try something like:
'{,bin/,config/,grunttasks/,lib/**/,test/**/,!test/lib/blanket}*.js'

Or maybe split it into two things:

'{,bin/,config/,grunttasks/,lib/**/,test/**/}*.js', '!test/lib/blanket.js'

Or if you're saying it's because of the copyright task, exclude the blanket.js file in grunttasks/copyright.js:

src: [ '<%= eslint.files %>', '!test/lib/blanket.js' ]

exclude the blanket.js file in grunttasks/copyright.js

👍 thank-you your globbiness, I shall follow this path.

shane-tomlinson · 2015-09-22T15:02:01Z

lib/config.js

+    api_timeout: {
+      doc: 'Timeout for talking to the Basket API server, in ms',
+      format: Number,
+      default: 5 * 1000 // 5 seconds


We talked about using the 5 seconds format in another repo, applicable here?

nice catch, done

shane-tomlinson · 2015-09-22T15:04:57Z

lib/logging/summary.js

+    var startTime = Date.now();
+    onFinished(res, function logSummary(err, res) {
+      var line = {
+        agent: req.headers['user-agent'],


I love that you alphabetized the keys.

shane-tomlinson · 2015-09-22T15:09:34Z

@rfk - there were a couple of outstanding items that @pdehaan initially brought up. I added a couple of notes, one about the config duration format, and .gitignore. All content server functional tests pass when using this repo from fxa-local-dev.

Once the few nits are either explicitly ignored or taken care of, merge away!

This commit imports the fxa-basket-proxy code from [1] as a standalone project, splitting it up into smaller chunks and adding some basic repo infrastructure and tests. The logic of the code itself is unchanged apart from some config-loading and logging tweaks. [1] https://github.com/mozilla/fxa-content-server/blob/13a59b2457d373190f1853c513604a5b72bb5106/server/bin/basket-proxy-server.js

rfk · 2015-09-23T19:03:11Z

Thanks @shane-tomlinson @pdehaan! Nits addressed, will merge when travis gives the final green light.

Split out fxa-basket-proxy into a standalone project; r=shane-tomlinson

rfk added the waffle:progress label Sep 10, 2015

rfk self-assigned this Sep 10, 2015

rfk added this to the FxA-44: journeybuilder events milestone Sep 10, 2015

pdehaan reviewed Sep 10, 2015
View changes

lib/basket/index.js

return {

status: 'error',

desc: String(desc),

code: code || module.exports.UNKNOWN_ERROR

This comment has been minimized.

Sign in to view

pdehaan Sep 10, 2015

module.exports.errors.UNKNOWN_ERROR?

pdehaan reviewed Sep 10, 2015
View changes

.eslintignore

.tmp/**

app/bower_components/**

app/scripts/vendor/**

dist/**

This comment has been minimized.

Sign in to view

pdehaan Sep 10, 2015

I don't know if any of these paths apply to this repo. We may be able to nuke this file.

pdehaan reviewed Sep 10, 2015
View changes

README.md

| TASK | DESCRIPTION |

|------|-------------|

| `grunt lint` | run ESLint, JSONLint, and JSCS (code style checker) on the code. |

This comment has been minimized.

Sign in to view

pdehaan Sep 10, 2015

Technically I don't think we're using JSONLint anywhere here.

pdehaan reviewed Sep 10, 2015
View changes

package.json

"mocha-text-cov": "^0.1.0",

"nock": "2.10",

"proxyquire": "1.6.0",

"sinon": "1.15.4",

This comment has been minimized.

Sign in to view

pdehaan Sep 10, 2015

I don't think we're using proxyquire or sinon anywhere in these files.

rfk force-pushed the initial-import-and-refactor branch Sep 11, 2015

rfk force-pushed the initial-import-and-refactor branch 2 times, most recently Sep 11, 2015

rfk added the waffle:review label Sep 11, 2015

rfk assigned zaach and unassigned rfk Sep 11, 2015

pdehaan reviewed Sep 11, 2015
View changes

rfk removed the waffle:progress label Sep 14, 2015