efcore icon indicating copy to clipboard operation
efcore copied to clipboard

Add `migrations has-pending-model-changes` Command

Open justindbaur opened this issue 2 years ago • 6 comments

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.

justindbaur avatar Jun 30 '23 17:06 justindbaur

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 add be better?
  • Should this have a PMC equivalent?
  • Should it support --json?

bricelam avatar Jul 07 '23 20:07 bricelam

You can add tests to Tests/Design/Internal/MigrationsOperationsTest.cs. See the ones in DbContextOperationsTest for a better example.

bricelam avatar Jul 07 '23 21:07 bricelam

📝 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 HasPendingModelChanges that returns a bool
  • 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)

bricelam avatar Jul 10 '23 21:07 bricelam

@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 avatar Jul 10 '23 21:07 bricelam

@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.

justindbaur avatar Jul 10 '23 21:07 justindbaur

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.

justindbaur avatar Jul 14 '23 17:07 justindbaur

Merged! Thanks, @justindbaur. I rebased and made a few minor tweaks

bricelam avatar Jul 18 '23 17:07 bricelam

@bricelam Thank you so much for taking it across the finish line. Can't wait to add this to our CI!

justindbaur avatar Jul 19 '23 00:07 justindbaur