-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/multiproxy migration #1651
Conversation
Whether it would be nicer to have control as a maintainer to decide when to run migrations, command line usage is still possible with external script: Now migrations run in startup always. |
Move the migrations folder into core. |
Remove console logs. We should consider storing log statements using some other method later. For now, we are avoiding console statements. |
const ApiBackends = new Mongo.Collection('apiBackends'); | ||
|
||
// Iterate through apiBackends collection | ||
ApiBackends.find().forEach((apiBackend) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reduce the number of characters here, and improve readability, rename apiBackend
to api
inside of the forEach()
loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, make sure that ApiBackends.find()
returns what you are expecting (an array of API Backends). Collection.find() typically returns a collection cursor, while Collection.find().fetch()
will return an array of results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brylie I iterate through old apiBackends collection, and thought "apiBackend" is different from our new "api". You want different naming for those?
"apiBackend" -> "api" and "api" -> "apiMigration" ?
// Construct apiUrl | ||
const apiUrl = `${apiBackend.backend_protocol}://${apiBackend.backend_host}`; | ||
// New api object | ||
const api = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With respect to the above comment, rename this variable as apiMigration
or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That way, lines 51-64 can be a bit shorter.
|
||
// Insert migrated api, get Id | ||
const apiId = Apis.insert(api); | ||
console.log(`Api ${apiId} inserted`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove console.log()
.
|
||
// Insert migrated proxyBackend | ||
const proxyBackendId = ProxyBackends.insert(proxyBackend, { validate: false }); | ||
console.log(`ProxyBackend ${proxyBackendId} inserted`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove console.log()
This looks really good. The only feedback is minor, to improve readability and follow our linting guidelines. |
const ApiBackends = new Mongo.Collection('apiBackends'); | ||
|
||
// Iterate through apiBackends collection | ||
ApiBackends.find().forEach((apiBackend) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, make sure that ApiBackends.find()
returns what you are expecting (an array of API Backends). Collection.find() typically returns a collection cursor, while Collection.find().fetch()
will return an array of results.
Double check that |
@brylie https://docs.mongodb.com/manual/reference/method/cursor.forEach/ It worked for me, as expected |
Alright. Move migrations into core, and we can merge. |
@brylie Done. |
Closes #1436