efcore
efcore copied to clipboard
Add `migrations has-pending-model-changes` Command
Fixes #26348
- [x] I've read the guidelines for contributing and seen the walkthrough
- [x] I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
- [x] The code builds and tests pass locally (also verified by our automated build checks)
- [x] Commit messages follow this format:
- [x] Tests for the changes have been added (for bug fixes / features)
- [x] Code follows the same patterns and style as existing code in this repo
I could not find anything that tests MigrationOperations methods or anything that directly tests the CLI. Please let me know if I missed anything and I'd be happy to write some tests.
My biggest question is the code I added in MigrationsOperations. It seems like a bit to much code for what normally goes in those methods but I don't know where the ideal location for this logic would sit. The code I wrote is a combination of the existing code here and from the comment on the issue here.
Also feel free to let me know the exact wording you would like for the resource strings, I doubt what I have there is great.
Open questions:
- What should the name of this command be?
dbcontext migration-pending? - Should we implement context.Database.HasChangesForAddMigration() at the same time and have it call that? (tentative method name)
- What should this output? Should it summarize the changes pending? Would a --dry-run flag on
migrations addbe better? - Should this have a PMC equivalent?
- Should it support --json?
You can add tests to Tests/Design/Internal/MigrationsOperationsTest.cs. See the ones in DbContextOperationsTest for a better example.
📝 Design Meeting Notes
- The command should be called
dotnet ef migrations has-pending-model-changes - The logic for this should be implemented as an extension method in RelationalDatabaseFacadeExtensions called
HasPendingModelChangesthat returns abool - A corresponding PMC command, and the --json option should wait for more/actual user requests before implementing them
- The command should always output some message saying whether or not there are changes (as already requested in PR review)
- A summary of changes is not useful/actionable (just run migrations add)
@justindbaur Alright, the comments should contain all the changes we'd like to see before mering this. Let us know if you have any follow up questions. It's looking really good.
@bricelam Thank you so much, I did start trying to write some tests but was having some issues because MockAssembly.Name != typeof(TestContext). Assembly.Name but maybe with making it an extension method off DatabaseFacade makes writing a test a bit easier. But thanks for the feedback, I'll hopefully have something ready for a second pass in the next couple days.
Hi @bricelam I believe I have addressed all feedback, the tests are not the prettiest things I have ever written. I had a bit of a learning curve trying to figure out how a snapshot model and design time model are created so I could best fake them while still achieving a high probability that the test is actually testing something. Let me know if you have a better way of achieving what I did.
Merged! Thanks, @justindbaur. I rebased and made a few minor tweaks
@bricelam Thank you so much for taking it across the finish line. Can't wait to add this to our CI!