Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

assemble all db repos #56

Merged
merged 2 commits into from
Jul 28, 2015
Merged

assemble all db repos #56

merged 2 commits into from
Jul 28, 2015

Conversation

dannycoates
Copy link
Contributor

fixes #54

New layout:

The db-server dep is moved in at the root level and required as a dependency by package.json and the mem bits are now intermixed in bin and lib. This allows everything to be in sync while allowing us to publish fxa-auth-db-server as a module to npm for 3rd party implementations.

Eventually we might want to rename this repo to reflect that its not just mysql, but its no big deal to leave it, and nothing will need to change for fxa-dev or prod. I've already got this running locally with fxa-auth-server with only a few minor changes.

├── CHANGELOG
├── Dockerfile
├── Gruntfile.js
├── LICENSE
├── README.md
├── bin
│   ├── db_patcher.js
│   ├── mem.js
│   └── server.js
├── config
│   ├── config.js
│   └── index.js
├── coverage.html
├── fxa-auth-db-server
│   ├── AUTHORS
│   ├── CHANGELOG
│   ├── CONTRIBUTING.md
│   ├── Gruntfile.js
│   ├── LICENSE
│   ├── README.md
│   ├── coverage.html
│   ├── docs
│   ├── grunttasks
│   ├── index.js
│   ├── lib
│   ├── package.json
│   ├── scripts
│   └── test
├── index.js
├── lib
│   ├── db
│   ├── logging.js
│   ├── notifier.js
│   └── promise.js
├── npm-shrinkwrap.json
├── package.json

Note, this requires npm >= 2.0 for the "file" type dependency.

@jrgm
Copy link
Contributor

jrgm commented Jun 18, 2015

Looks good. But looks like you'll need to npm install -g npm@2 in the .travis.yml because the 0.10 build is using npm @1.4.28 unfortunately.

@dannycoates dannycoates force-pushed the avengers branch 2 times, most recently from 00e5550 to 7ca9ea1 Compare June 18, 2015 03:10
@dannycoates
Copy link
Contributor Author

Thanks @jrgm, now we're green

@dannycoates
Copy link
Contributor Author

maybe it would be less weird if instead of the npm dep if we just changed the requires. I was dazzled by the new shiny 'file:' option.

@dannycoates
Copy link
Contributor Author

maybe it would be less weird

except then we'd need to promote the db-server deps up. clearly i need to stop looking at this. 🍺

@chilts
Copy link
Contributor

chilts commented Jul 9, 2015

Things I've looked at which seem fine:

  • npm test in the top level seems fine (including patching the database)
  • npm install && npm test in fxa-auth-db-server seems fine
  • node bin/mem.js seems fine
  • npm start starts a mysql server fine

Random minor queries:

  • db_patcher.js is now executable but is only ever run using node (doesn't have a #! interpreter) ... minor, but is it needed?
  • I think the tests are running the mysql backend (test/backend/db_tests.js requires '../../lib/db/mysql' same in test/backend/remote.js) but neither are doing the same with the mem backend, is that true? If so, should it do both? This is probably my main concern here. 🌵

Apart from delving into all of the code (which just looks like a re-arrangement with probable changes to requires) then this all looks fine to me! :) r+ with nit regarding backend tests. Totally bogus @dannycoates!

bogus

@@ -0,0 +1,39 @@
language: node_js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this .travis.yml in a random subdirectory actually get run?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

@rfk
Copy link
Contributor

rfk commented Jul 9, 2015

I think the tests are running the mysql backend (test/backend/db_tests.js requires '../../lib/db/mysql'
same in test/backend/remote.js) but neither are doing the same with the mem backend,

If true, we need to figure out a way to run both sets of tests under travis

@rfk rfk assigned dannycoates and unassigned chilts Jul 21, 2015
@rfk
Copy link
Contributor

rfk commented Jul 21, 2015

IIUC the next step here is for danny to comment on chilts' queries above, then we can rebase for a merge.

@rfk
Copy link
Contributor

rfk commented Jul 28, 2015

I went ahead and rebased this and merged in train-42 from the other repos. Let's get it merged and we can file follow-up bugs for further cleanup. Merging r=@chilts

rfk added a commit that referenced this pull request Jul 28, 2015
@rfk rfk merged commit 46e2624 into master Jul 28, 2015
@dannycoates
Copy link
Contributor Author

Eh, sorry I haven't looped back to this yet. @rfk, thanks for the merge anyway. I see @philbooth took the followups, lmk if I can help with anything :)

@shane-tomlinson shane-tomlinson deleted the avengers branch April 18, 2018 12:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor our fragile cross-repo dependency setup
4 participants