feat: add DB migration flow #164
Conversation
bsatrom
force-pushed
the
task/db-migration
branch
from
9522725
to
b46cb60
Compare
bsatrom
force-pushed
the
task/db-migration
branch
from
b46cb60
to
6957027
Compare
lib/controllers/mongo.js
Outdated
| hostname: api[1], | ||
| port: api[0] === 'https' ? 443 : 80, | ||
| version: 'v2', | ||
| token: null |
There was a problem hiding this comment.
why not add the user token here?
There was a problem hiding this comment.
same comment as above. I assumed this was for an admin token, but if putting the apiKey here prevents the need to pass in that second param, I can include it
lib/controllers/mongo.js
Outdated
| var Client = apiUtil({ | ||
| protocol: api[0], | ||
| hostname: api[1], | ||
| port: api[0] === 'https' ? 443 : 80, |
There was a problem hiding this comment.
i don't think the port is necessary to set. it will be 443. If not, there are other configuration problems that exist.
There was a problem hiding this comment.
going off the docs in the api-util repo, but I can remove it if it's not needed
lib/controllers/mongo.js
Outdated
| Client.post('/databases/' + db.id + '/clone', { | ||
| db: db, | ||
| hosts: hosts | ||
| }, userConfig.data.apiKey, callback) |
There was a problem hiding this comment.
if the token is added to the Client.token then it can be removed here
lib/routes/mongo.js
Outdated
| this.line('Migrates a MongoDB database to a different version and host.'.input) | ||
| }) | ||
|
|
||
| xervo.program |
There was a problem hiding this comment.
Since there's a convention of having flags for anything besides the command, is it best to do so here? It would also make the .action function not require another function that calls a function
package.json
Outdated
| @@ -32,6 +32,7 @@ | |||
| } | |||
| ], | |||
| "dependencies": { | |||
| "@xervo/api-util": "^0.2.0", | |||
lib/commands/env.js
Outdated
| if (err) { | ||
| return error.handlePromptError(err, cb) | ||
| } | ||
| return cb(null, envVars[result.environmentVar - 1]) |
There was a problem hiding this comment.
no need to return here since it's the last call
lib/commands/mongo.js
Outdated
| } | ||
| } | ||
| if (databases.length === 0) { | ||
| xervo.io.error('You currently have no databases. One can be created using the `mongo create` command.') |
There was a problem hiding this comment.
You can actually pass that message to the callback as the error and it will print. Saves 1 line of code 🥂
lib/commands/mongo.js
Outdated
| } | ||
|
|
||
| if (databaseName !== '') { | ||
| for (var i = 0; i < databases.length; i++) { |
There was a problem hiding this comment.
You could use databases.find instead of looping over each one individually
lib/commands/mongo.js
Outdated
|
|
||
| mongo.chooseDatabasePrompt(databases, databaseName, function (err, database) { | ||
| if (err) { | ||
| err = error.handleApiError(err, 'MIGRATE_MONGO', cb) |
There was a problem hiding this comment.
this would be a prompt error instead of api error
lib/commands/mongo.js
Outdated
|
|
||
| mongo.chooseRegionPrompt(function (err, regionData) { | ||
| if (err) { | ||
| err = error.handleApiError(err, 'MIGRATE_MONGO', cb) |
There was a problem hiding this comment.
if mongo.chooseRegionPrompt handles the error, you can just pass it to the callback instead of doing another error handler
| xervo.io.success('Database available at: ' + database.uri + '. Please update any running applications to use this db before removing your original.') | ||
|
|
||
| // Create users for the new DB | ||
| createUserOnClonedDB(database, false, function () { |
There was a problem hiding this comment.
do you need to handle errors here?
Initial flow for DB migration added under
xervo mongo migratecommand. Works up tolibrarianmigrate call.addresses XervoIO/product-dev/issues/10