-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(mojaloop/#3139)!: rework config to accept env variables for DB #132
Conversation
7ca8711
to
7b0a28f
Compare
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.
Snapshot is working fine for me.
Only suggested change is on the "format" for the port, which should be a number?
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.
Some additional comments for your attention.
src/shared/config-db.ts
Outdated
}, | ||
timezone: { | ||
doc: 'Database timezone', | ||
format: '*', |
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.
Do they have a format for timezone? or perhaps this should be an enum?
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.
No and probably not. Since knex
services connecting to multiple flavors of databases I think the format can be a varied set of strings.
Updated it to be a String
@mdebarros addressed comments. |
CI checks are failing. |
src/shared/config-db.ts
Outdated
format: DbPoolFormat.name, | ||
default: null | ||
min: { | ||
doc: 'Database timezone', |
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.
Doc discriptions look like copy-pasta?
I think they all need to be updated to match the config pool paramater's description.
Same comment applies to other connection-pool configs.
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.
+1
feat(mojaloop/#3139): rework config to accept env variables for DB - mojaloop/project#3139
'memory'
for sqllite for testing which took a string but now it can take an object.