Skip to content

Add migrations has-pending-model-changes Command#31164

Merged
bricelam merged 2 commits intodotnet:mainfrom
justindbaur:main
Jul 18, 2023
Merged

Add migrations has-pending-model-changes Command#31164
bricelam merged 2 commits intodotnet:mainfrom
justindbaur:main

Conversation

@justindbaur
Copy link
Contributor

@justindbaur justindbaur commented Jun 30, 2023 •
edited
Loading

Fixes #26348

  • I've read the guidelines for contributing and seen the walkthrough
  • 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
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
  • Tests for the changes have been added (for bug fixes / features)
  • 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 justindbaur mentioned this pull request Jun 30, 2023
Closed
bricelam
bricelam reviewed Jul 7, 2023
View reviewed changes
bricelam
bricelam reviewed Jul 7, 2023
View reviewed changes
@bricelam
Copy link
Contributor

bricelam commented Jul 7, 2023 •
edited
Loading

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
bricelam suggested changes Jul 7, 2023
View reviewed changes
@bricelam
Copy link
Contributor

bricelam commented Jul 7, 2023

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

@bricelam
Copy link
Contributor

bricelam commented Jul 10, 2023

📝 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
Copy link
Contributor

bricelam commented Jul 10, 2023

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

@justindbaur
Copy link
Contributor Author

justindbaur commented Jul 10, 2023

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

@bricelam bricelam mentioned this pull request Jul 10, 2023
Open
@justindbaur justindbaur changed the title Add migrations check-pending Command Add migrations has-pending-model-changes Command Jul 11, 2023
@justindbaur
Copy link
Contributor Author

justindbaur commented Jul 14, 2023

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 justindbaur requested a review from bricelam July 14, 2023 17:18
bricelam
bricelam reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@justindbaur @bricelam
Add migrations has-pending-model-changes Command
20372c5 
- Add command
- Add DatabaseFacade extension method
bricelam added a commit to justindbaur/efcore that referenced this pull request Jul 18, 2023
@bricelam
Clean up PR dotnet#31164
43aeb62
@bricelam bricelam merged commit 5ac0582 into dotnet:main Jul 18, 2023
@bricelam
Copy link
Contributor

bricelam commented Jul 18, 2023

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

@justindbaur
Copy link
Contributor Author

justindbaur commented Jul 19, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@bricelam bricelam bricelam approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bricelam bricelam

Labels

community-contribution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

How to do a missing migration check in CI

3 participants

@justindbaur @bricelam @ajcvickers