Skip to content
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

Merged
merged 16 commits into from
Feb 28, 2023

Conversation

kleyow
Copy link
Contributor

@kleyow kleyow commented Feb 24, 2023

feat(mojaloop/#3139): rework config to accept env variables for DB - mojaloop/project#3139

  • Update db connection config to be set with ENV variables
  • Update to new auditing library and flow
  • Resolve jest upgrades. It was the main solution to errors I was facing with axios and hapi dependency bumps.
  • Change database connection config to object instead of object | string. We used 'memory' for sqllite for testing which took a string but now it can take an object.
  • Update dependencies

@kleyow kleyow changed the title Feat/#3139 env set feat(mojaloop/#3139): rework config to accept env variables for DB Feb 24, 2023
@kleyow kleyow force-pushed the feat/#3139-env-set branch from 7ca8711 to 7b0a28f Compare February 27, 2023 03:28
@kleyow kleyow marked this pull request as ready for review February 27, 2023 03:45
@kleyow kleyow changed the title feat(mojaloop/#3139): rework config to accept env variables for DB feat(mojaloop/#3139)!: rework config to accept env variables for DB Feb 27, 2023
Copy link
Member

@mdebarros mdebarros left a 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?

src/shared/config-db.ts Outdated Show resolved Hide resolved
src/shared/config-db.ts Outdated Show resolved Hide resolved
src/shared/config-db.ts Outdated Show resolved Hide resolved
Copy link
Member

@mdebarros mdebarros left a 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 Show resolved Hide resolved
src/shared/config-db.ts Outdated Show resolved Hide resolved
},
timezone: {
doc: 'Database timezone',
format: '*',
Copy link
Member

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?

Copy link
Contributor Author

@kleyow kleyow Feb 28, 2023

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

@kleyow
Copy link
Contributor Author

kleyow commented Feb 28, 2023

@mdebarros addressed comments.

@mdebarros
Copy link
Member

@mdebarros addressed comments.

CI checks are failing.

format: DbPoolFormat.name,
default: null
min: {
doc: 'Database timezone',
Copy link
Member

@mdebarros mdebarros Feb 28, 2023

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.

Copy link
Member

@mdebarros mdebarros left a comment

Choose a reason for hiding this comment

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

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants