POC - Just for discussion - Move all runner code except DB specific dependencies into FluentMigrator.Runner.Core #1198
Conversation
|
I'm going to start reviewing this next weekend (4/11-4/12). Thanks for putting this together, @gliljas ! |
|
Hey @gliljas I thought about this idea some more and I really like it. The economic value it adds is:
Previously, nobody could give me great reason to do this other than "I hate files in my directory". Remaining proof-of-concept is re-bundling FluentMigrator.Console and verifying it ships all runners still. I am assuming you expect FluentMigrator.DotNet.Cli to also ship everything, and that this change is mainly to support using the In-Process Runner? |
|
Yes, the console app could/should definitely be a more complete package. |
|
Yes. Realistically it might take me a few months to factor in this change, as I'm working on restructuring all the tests so that they're split out per runner as well, which is a ton of work. I'm not a huge fan of injecting all the runners into a single test. Ideally, there should be a driver program that accepts a runner config as an injected parameter, and the SUT is the path through the runner we wish to exercise. My thought is that I could then generate a report for which runners cover which "test drivers", and have an "architecture verification test" for saying things like "SQLite doesn't support partitions, so ignore tests related to partitions and partitioned indexes" and "SQL Server seems to be missing tests for partitioned indexes but has create/alter partition tests." |
There was a problem hiding this comment.
@gliljas I finally got around to wrapping my head around this large PR. Sorry it took so long. I definitely think we should make the runner abstraction in Core more explicit and not include all driver dependencies in core.
I think one thing that's not clear to me is what happens if you upgrade to FluentMigrator 3.2.8 locally but your build server is on 3.2.1. I believe the behavior will depend on RollForward behavior, and that any runtime dependency mismatches will not get caught until you go to production. Do you have any suggestions on how we can detect such mismatches and avoid them? See #1211 for an example where a customer was bitten by this. I can see the argument this is an orthogonal issue, but my concern is once we make the dependencies injectable, I will get way more support requests as the average developer is not aware how .NET loads libraries at run-time and the problems with attribute-based code discovery. Long term, ideally, MigrationAttribute would have an interface and people could create their own attributes and attribute filtering logic. This would force people to statically reference the assembly containing the attribute definition logic, and create a run-time error as opposed to silent error. The run-time error would then fail when it first attempts to discover migrations, as opposed to some point in the run-time chain.
| @@ -40,10 +40,6 @@ static int Main(string[] args) | |||
| var app = new CommandLineApplication(); | |||
|
|
|||
| var help = app.HelpOption(); | |||
| var mode = app.Option<MigrationMode>( | |||
There was a problem hiding this comment.
@gliljas Why is this a pre-requisite to achieving this change?
There was a problem hiding this comment.
To be honest, I'm not sure. I believe it was just a victim of making things compile, i.e no legacy stuff.
| var dbConfig = CreateDatabaseConfiguration(processor, connectionString); | ||
| switch (selectedMode) |
There was a problem hiding this comment.
@gliljas See above. Not clear on why this needs to be removed? I looked at RunInLegacyMode and it doesnt seem to contain any concrete DB driver dependencies? RunWithServices below does, but that should be addressed by creating interfaces and letting people try to register a type if it exists.
The downside I see to hot loading types in RunWithServices is that you move away from deps.json auto-resolving the dependency, so you can end up in situations where a developer has upgraded to a newer version of fluentmigrator, but the build ci/deploy cd servers are on older versions, and thus you get a runtime error due to not resolving dependencies.
| @@ -9,6 +9,38 @@ | |||
| <ItemGroup Condition=" '$(TargetFramework)' == 'net461' "> | |||
| <Reference Include="System.Configuration" /> | |||
| </ItemGroup> | |||
| <ItemGroup> | |||
There was a problem hiding this comment.
What's the purpose of this change? It removes 10 lines and adds 30, and it's not clear that it's solving any particular problem?
| services | ||
| .AddLogging(lb => lb.AddProvider(new LegacyFluentMigratorLoggerProvider(runnerContext.Announcer))) | ||
| .AddFluentMigratorCore() | ||
| .AddAllDatabases() |
There was a problem hiding this comment.
Note to self: This can't be called from FluentMigrator.Runner.Core as its been moved to src/FluentMigrator.Runner/FluentMigratorServiceCollectionExtensions.cs and doing so would create a circular dependency.
| /// </summary> | ||
| /// <param name="services">The service collection</param> | ||
| /// <returns>The service collection</returns> | ||
| private static IServiceCollection AddAllDatabases(this IServiceCollection services) |
There was a problem hiding this comment.
Note to self: Moved to src/FluentMigrator.Runner/FluentMigratorServiceCollectionExtensions.cs
| /// <param name="assembly">The assembly to scan for migrations, etc...</param> | ||
| /// <param name="runnerContext">The runner context</param> | ||
| /// <param name="processor">The migration processor</param> | ||
| [Obsolete] |
There was a problem hiding this comment.
Why is this required to be removed? Removing it means we would have to make this a 4.0 release, right?
| /// <param name="runnerContext">The runner context</param> | ||
| /// <param name="processor">The migration processor</param> | ||
| /// <param name="conventionSet">The expression convention set</param> | ||
| [Obsolete] |
There was a problem hiding this comment.
Why is this required to be removed? Removing it means we would have to make this a 4.0 release, right?
| /// <param name="processor">The migration processor</param> | ||
| /// <param name="versionTableMetaData">The version table metadata</param> | ||
| /// <param name="migrationRunnerConventions">The custom migration runner conventions</param> | ||
| [Obsolete] |
There was a problem hiding this comment.
Why is this required to be removed? Removing it means we would have to make this a 4.0 release, right?
| /// <param name="migrationRunnerConventions">The custom migration runner conventions</param> | ||
| /// <param name="conventionSet">The expression convention set</param> | ||
| /// <param name="migrationScopeHandler">The migration scope handler</param> | ||
| [Obsolete] |
There was a problem hiding this comment.
Why is this required to be removed? Removing it means we would have to make this a 4.0 release, right?
| @@ -60,42 +60,6 @@ public class VersionLoader : IVersionLoader | |||
| public IMigration VersionUniqueMigration { get; } | |||
| public IMigration VersionDescriptionMigration { get; } | |||
|
|
|||
| [Obsolete] | |||
There was a problem hiding this comment.
Why is this required to be removed? Removing it means we would have to make this a 4.0 release, right?
|
@gliljas @jzabroski |
|
@SomebodyOdd Not a priority right now. I am trying to true up FluentMigrator develop and master branches and end FluentMigrator v3.x so we can focus on 4.x and onwards. There is pretty significant drift between the two branches and now that .NET 5 is released, that needs to be the main focus. I think once we clean up and retire the develop branch, we can more easily integrate a change like this (which would be a massive breaking change). It would really discourage me personally from ever syncing develop and master if we added a change like this now. #1348 contains the spreadsheet where I have tried to figure out this drift that started with v3 release. |
|
To be clear, I do want to do this - along with Async All the Way, and a few other features. I just have a roadmap in my head of how to get there most efficiently and how to value my free time such that I don't get discouraged along the way. |
|
@jzabroski Got it, thanks |
|
@jzabroski is there anything you need to help with to get this over the line? I was contemplating re-submitting it and seeing if we can re-instate much of the deleted code as this appears to be the greatest concern in the code review. Is there anything else that is causing a roadblock? |
|
@jzabroski I thought I'd take a look to see how hard it would be to just move all the code with no deletions and I managed to get it to build and have all tests pass. I've re-submitted it as a new PR #1600 |
|
@gliljas I will close this POC. I will work with @mattbrailsford on finally delivering this feature request for 4.0. |
That's super! |
This leaves FluentMigrator.Runner as a meta package.
Getting this to work with obsolete code was not worth it in a POC, so I removed it. Ran the integration tests for SQL Server 2016. All green.