erikbra / grate

grate - the SQL scripts migration runner
MIT License
209 stars 40 forks source link

Suppressing logging of internal grate migrations doesn't appear to be working correctly #519

Closed JaDuyve closed 6 months ago

JaDuyve commented 6 months ago

Describe the bug Grate still logs everything while running grate internal migrations.

From debugging the code I think the reason it still logs everything when running internal migrations is because the logger is created before setting the new configuration into the DbMigrator instance. The method IsInternalMigration uses the configuration from inside the DbMigrator instance as you can see inside code snippet 2.

https://github.com/erikbra/grate/blob/13132276d62123b21946b436d81bc63d1b6b7ee5/src/grate.core/Migration/GrateMigrator.cs#L16-L31

https://github.com/erikbra/grate/blob/13132276d62123b21946b436d81bc63d1b6b7ee5/src/grate.core/Migration/GrateMigrator.cs#L282-L283

I tried lowering the line where the logger is created below the line where the new configuration (value) is set into the newly created DbMigrator instance, and it worked from what I can see. Although still some things are logged because they are logged with a logger provided by dependency injection and not the custom created logger (I'm not sure if this is a bad thing).

This is what I get when I apply the above-mentioned change.

Initializing connections.
Running grate v1.0.0 (build date 05/12/2024 17:58:26) against localhost,1435 - TestDb.
Looking in D:\Repos\TestDb for scripts to run.
================================================================================
Setup, Backup, Create/Restore/Drop
================================================================================
================================================================================
Grate Structure
================================================================================
Initializing connections.
 Versioning TestDb database with version 1.0.0.
Initializing connections.
 Versioning TestDb database with version 1.0.0.
Initializing connections.
 Versioning TestDb database with version 1.0.0.
Initializing connections.
 Versioning TestDb database with version 1.0.0.
================================================================================
Versioning
================================================================================
 Migrating TestDb from version 0.0.0.0 to 0.0.0.1.
 Versioning TestDb database with version 0.0.0.1.
================================================================================
Migration Scripts
================================================================================
Skipping 'BeforeMigration', beforeMigration does not exist.
Skipping 'AlterDatabase', alterDatabase does not exist.
Skipping 'Run After Create Database', runAfterCreateDatabase does not exist.
Skipping 'Run Before Update', runBeforeUp does not exist.

Looking for Update scripts in "D:\Repos\TestDb\./ddl/05.up.OneTime". These should be one time only scripts.
--------------------------------------------------------------------------------
  Running '20240312_01_CreateTables.sql'.
--------------------------------------------------------------------------------

Skipping 'Run First After Update', runFirstAfterUp does not exist.
Skipping 'Functions', functions does not exist.
Skipping 'Views', views does not exist.
Skipping 'Stored Procedures', sprocs does not exist.
Skipping 'Triggers', triggers does not exist.
Skipping 'Indexes', indexes does not exist.
Skipping 'Run after Other Anytime Scripts', runAfterOtherAnyTimeScripts does not exist.
Skipping 'Permissions', permissions does not exist.
Skipping 'AfterMigration', afterMigration does not exist.

grate v1.0.0 (build date 05/12/2024 17:58:26) has grated your database (TestDb)! You are now at version 0.0.0.1. All changes and backups can be found at "C:\Users\jaduyve\AppData\Local\grate\migrations\TestDb\localhost_1435\2024-05-12T17_58_29.6460636_02_00".

To Reproduce Just run grate when the internal migrations haven't been run.

Expected behavior Logs with log level below warning during internal migration should not be visible.

Desktop (please complete the following information):

JaDuyve commented 6 months ago

I created a PR which applies the above-mentioned change. Do you think this change would be suitable for this issue 😃? https://github.com/erikbra/grate/pull/520

erikbra commented 6 months ago

Thank you. Bad testing on my part there 😄 unit tests doesn't test everything...

erikbra commented 6 months ago

Fixed in #520