Move all FluentMigration.Runner code to FluentMigrator.Runner.Core #1600
Conversation
…cept for a single extension method for adding all databases
|
This seems nice, based on your comments/description of the changes. I have not looked at the code changes yet. I'm in the process of updating the example Runner. - In particular, I am cherry-picking 369037c from the Ideally, we also tackle #1006 , in particular #1208 - See my comments here: #1208 (comment) Fully closing #1006 would probably entail smoothing out how we package FluentMigrator.MSBuild, as the way it is bundled today is incomplete and there should probably be a second "batteries included" version of FluentMigrator.MSBuild package. I think we may want to |
|
I think we should update: to include a warning message (in yellow ANSI Color code) at the entry point: System.Console.WriteLine($"WARN: FluentMigrator.Console is obsolete, and will be going away in 5.0.0 release.{System.Environment.NewLine}Please check the upgrade guide http://fluentmigrator.github.io/....");Something like that. That can go in a separate ticket. |
|
Additional Notes to self: Pull over my comments from #1198
and
along with:
I gave this a lot of thought over the years. Perhaps for 4.0 we not address it. Ideally we can analyze the deps. I believe .NET 6.0 has APIs to do this. |
|
Not sure if there is anything you want me to action in there, or if it's just notes for your own review, but let me know if there is anything you need me to implement, otherwise I'll wait for a code review. If based on your comments though this is already being tackled in another area, it would be good if some info could be placed somewhere as it's hard to track the status of the project and where to direct efforts to assist. I wonder if some kanban board or something is needed to know the status of the next release and what is still required to be done and how anyone can help, and if anything is "up for grabs". |
|
These are just notes for me to move into documentation/roadmap pages.
No, this PR is needed. The notes are just the "economic benefits" and what this would enable long-term, plus a few minor dependency issues I am aware of that are tangentially related. |
Something that would be "up for grabs" is looking at #1348 , in particular the spreadsheet I compiled, and taking all the Snowflake commits from the develop branch and getting them into master for 4.0. It's mostly busy work, but I have my hands full with work, running a company, two open source projects, kid on the way, etc. In most cases I know exactly what should be done with this project, it's just finding time to go do it. |
|
Additional note: If we're going to break this apart, we need document how to load the SNI (Server Name Indication) package, or people are probably going to file time consuming bugs. |
|
Spoke to Simon Cropp. He told me a lot of popular nuget packages, like Newtonsoft JSON and Serilog, lock the assembly version at the major version number, even though the package might use servicing releases in the SemVer. This will address #1211 mentioned above in this comment: #1600 (comment) @fubar-coder Thoughts on this particular solution to the "reflection can't find MigrationAttribute" issue when dynamically loading assemblies via out-of-process runners? |
|
@jzabroski This problem usually happens when you're referencing (and using) two different versions of the assembly containing the attribute, which means that the type information instance is different even though it's the same type/code (but from 2 different assemblies). There are two possible workarounds:
|
|
@mattbrailsford So, here are my takeaways from the PR (for release notes, and possibly questions/remarks): The following things are moving from FluentMigrator.Runner to FluentMigrator.Runner.Core
I'm not finished reviewing even the first commit yet, but these are my notes so I can tackle it piecemeal. |
|
Should wrap up reviewing this week. Got buried with life and work. |
| @@ -45,6 +46,9 @@ public static IMigrationRunnerBuilder AddSqlServer(this IMigrationRunnerBuilder | |||
| .AddScoped<IMigrationProcessor>(sp => sp.GetRequiredService<SqlServer2016Processor>()) | |||
| .AddScoped<SqlServer2016Generator>() | |||
| .AddScoped<IMigrationGenerator>(sp => sp.GetRequiredService<SqlServer2016Generator>()); | |||
|
|
|||
| MigrationProcessorFactoryProvider.Register(new SqlServerProcessorFactory()); | |||
There was a problem hiding this comment.
I think these should be unneeded. MigrationProcessorFactoryProvider has been marked as obsolete for some time now, and it really duplicates what DI can do.
| @@ -44,38 +32,7 @@ public class MigrationProcessorFactoryProvider | |||
|
|
|||
| [Obsolete] | |||
| static MigrationProcessorFactoryProvider() | |||
| { | |||
There was a problem hiding this comment.
I think we should just delete this class and ConnectionlessProcessorFactoryProvider - need to see what that impacts.
| /// <param name="runnerConventions">The runner conventions used to identify a version table metadata type</param> | ||
| /// <param name="runnerContext">The runner context defining the search boundaries for the custom version table metadata type</param> | ||
| /// <returns>A custom or the default version table metadata instance</returns> | ||
| public static Type GetVersionTableMetaDataType( |
There was a problem hiding this comment.
Why are these "LegacyExtensions"? Shouldn't there be a section explaining what to use? Shouldn't this be marked as Obsolete if its Legacy?
| /// <param name="assemblies">The assembly collection</param> | ||
| /// <param name="serviceProvider">The service provider to get all the required services from</param> | ||
| /// <returns>A custom or the default version table metadata instance</returns> | ||
| public static Type GetVersionTableMetaDataType( |
There was a problem hiding this comment.
Why are these "LegacyExtensions"? Shouldn't there be a section explaining what to use? Shouldn't this be marked as Obsolete if its Legacy?
| /// <param name="connectionStringProvider">The connection string provider</param> | ||
| /// <param name="runnerContext">The runner context</param> | ||
| /// <returns>The found connection string</returns> | ||
| public static string LoadConnectionString( |
There was a problem hiding this comment.
Why are these "LegacyExtensions"? Shouldn't there be a section explaining what to use? Shouldn't this be marked as Obsolete if its Legacy?
| /// <param name="loaderFactory">The factory to create an <see cref="IAssemblyLoader"/> for a given assembly (file) name</param> | ||
| /// <param name="assemblyNames">The assembly (file) names</param> | ||
| /// <returns>The collection of assemblies that could be loaded</returns> | ||
| public static IEnumerable<Assembly> GetTargetAssemblies( |
There was a problem hiding this comment.
Why are these "LegacyExtensions"? Shouldn't there be a section explaining what to use? Shouldn't this be marked as Obsolete if its Legacy?
| /// </summary> | ||
| /// <param name="assemblies">The assemblies to get the exported types from</param> | ||
| /// <returns>The exported types</returns> | ||
| public static IReadOnlyCollection<Type> GetExportedTypes(this IEnumerable<Assembly> assemblies) |
There was a problem hiding this comment.
Why are these "LegacyExtensions"? Shouldn't there be a section explaining what to use? Shouldn't this be marked as Obsolete if its Legacy?
|
@mattbrailsford OK, I think my review is pretty much done. Please take a look - it's pretty minor stuff I think, but just "fit and finish" polish before we release. |
|
@jzabroski just a couple of things
|
|
@mattbrailsford I see your point of view, and acknowledge your POV makes sense. |
|
@mattbrailsford I'll start drafting the release notes and will target 4.0 release this weekend. I don't want to drag it out seeking perfection. Plus, decoupling the runner is a big accomplishment |
Hi, what is the current state and do you have a plan when we can expect the 4.0 release? I know you are probably busy but the latest comment about the release is from June 7th. |
|
I need to release it. I just spent last weekend fixing fluentmigrator/documentation in prep for 4.0 release. |
|
Hello. |
|
I am waiting too |
|
I got tired of waiting and made this: https://www.nuget.org/packages/FluentMigratorBridge which lets you use just a single database runner instead of referencing them all. |
Is this a fork of 3.3.2? |
|
No, but it is compatible with FluentMigrator 3.3.2. It's a library that allows you to create migrations and a runner with just a single database, instead of pulling in all the libraries for all databases. Instructions are on the github repo. |
Starred the repo, amazing job @robertcoltheart thank you so much. Worked like a charmed. |
|
I should be pushing 4.0.0 shortly. |
This is a re-implementation of PR #1198 but does not remove any legacy / obsolete code. This should result in the same outcome, that FluentMigrator.Runner is now just a meta package with a single extensions method for adding all database types to the
IMigrationRunnerBuilderbut all other code has moved toFluentMigrator.Runner.Corethus meaning folks shouldn't need to install all database providers.The only real changes I've had to make are
AddAllDatabasesextension method inFluentMigrator.Runnerhas changed from being aIServiceCollectionextension method to anIFluentMigratorRunnerBuilderextension and has been made public.FluentMigratorServiceCollectionExtensions.CreateServicesmethod now no longer callsAddAllDatabasesand instead accepts an optionalAction<IMigrationRunnerBuilder> configureRunnerto allow external configuration and to call theAddAllDatabases()extensionTaskExecutornow accepts an optionalAction<IMigrationRunnerBuilder> configureRunnerthat is passed through to theCreateServicesmethod above.AddDbTypemethods have been updated to register the provider factory with theMigrationProcessorFactoryProviderrather than theMigrationProcessorFactoryProviderbeing responsible for adding them all.I guess something to clarify here is that I've only made the change such that it's possible to only depend on FluentMigrator.Runner.Core and the DB provider you need, but I haven't changed anything in the runner implementations (CLI, Console, etc) to allow you to only use what you need from there. To be honest, I don't know what these actually do / how they are used as I'm primarily using FluentMigrator as an in-app process and so I run the migrations via C# code so what I have implemented here is enough for my needs.
Maybe this PR can be accepted as a stepping stone and then review what can be done to the runner implementations such that those can also be configured to only require the dependencies they need.