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

OrmMigrationAndLogger #1988

Merged
merged 26 commits into from
Oct 18, 2024
Merged

OrmMigrationAndLogger #1988

merged 26 commits into from
Oct 18, 2024

Conversation

PooyaRaki
Copy link
Contributor

@PooyaRaki PooyaRaki commented Oct 1, 2024

Summary

Introduces a locking mechanism to TypeORM migrations to prevent conflicts when multiple instances attempt to run the same migration simultaneously.

Adds a custom query logger that logs queries to the console without including bound parameters.

Changes

  • Disables migrationsRun in the TypeORM settings.
  • Introduces a new query logger to TypeORM.
  • Ensures TypeORM migrations run after locking the database during startup.
  • If migrations are already running in another instance, it retries a few times to check if they have been completed successfully before throwing an error.

@PooyaRaki PooyaRaki force-pushed the ormMigrationAndLogger branch 2 times, most recently from 1f7a92b to 8cae51a Compare October 2, 2024 08:59
@PooyaRaki PooyaRaki marked this pull request as ready for review October 2, 2024 15:36
@PooyaRaki PooyaRaki requested a review from a team as a code owner October 2, 2024 15:36
Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

Regarding usage of "our" configu module. Perhaps we can reconsider usage of the "native" one instead. That should be done separately though.

src/config/entities/configuration.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/database-migrator.service.spec.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/database-migrator.service.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/database-migrator.service.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/postgres-database.service.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/postgresql-logger.service.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/postgresql-logger.service.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/postgresql-logger.service.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
@PooyaRaki PooyaRaki marked this pull request as ready for review October 15, 2024 16:30
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/app.module.ts Outdated Show resolved Hide resolved
src/config/entities/__tests__/configuration.ts Outdated Show resolved Hide resolved
src/config/entities/__tests__/configuration.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/postgres-database.service.spec.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/postgres-database.service.spec.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/postgres-database.service.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/postgresql-logger.service.ts Outdated Show resolved Hide resolved
migrations/1727702843994-notification_types.ts Outdated Show resolved Hide resolved
src/config/entities/__tests__/configuration.ts Outdated Show resolved Hide resolved
src/config/entities/configuration.ts Outdated Show resolved Hide resolved
src/datasources/db/v2/database-initialize.hook.ts Outdated Show resolved Hide resolved
@PooyaRaki
Copy link
Contributor Author

PooyaRaki commented Oct 16, 2024

@hectorgomezv

Yes, this change is due to the fact the database is requested by the controller tests when the v1 database module is attached to the AppModule.

Done 2ea0e5d

I'd love some documentation on these properties.

Since these are TypeOrm parameters I didn't add comments. Do you want me to add TypeOrm reference?

Exactly, the purpose of passing the loggingService is to use it for logging, instead of using the console. The monitoring tools we have in place consume JSON-formatted logs, so logging directly using the console would not play well with it. By using the loggingService it would send JSON-formatted logs, and will also add the appropriate metadata fields (version, build_number).

Done 2ea0e5d

If DatabaseInitializeHook does not contain any logic, I'd remove the class directly.

I added the initialize call for the sake of consistency.

@PooyaRaki PooyaRaki mentioned this pull request Oct 17, 2024
@PooyaRaki PooyaRaki merged commit 29fc38b into orm Oct 18, 2024
16 checks passed
@PooyaRaki PooyaRaki deleted the ormMigrationAndLogger branch October 18, 2024 08:43
PooyaRaki added a commit that referenced this pull request Oct 18, 2024
- Introduces a new query logger to TypeORM.
- Ensures TypeORM migrations run after locking the database during startup.
PooyaRaki added a commit that referenced this pull request Oct 22, 2024
- Introduces a new query logger to TypeORM.
- Ensures TypeORM migrations run after locking the database during startup.
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.

3 participants