Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jul 18, 2023
Merged

Conversation

justindbaur
Copy link
Contributor

@justindbaur justindbaur commented Jun 30, 2023

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.

@bricelam
Copy link
Contributor

bricelam commented Jul 7, 2023

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?

src/EFCore.Design/Properties/DesignStrings.resx Outdated Show resolved Hide resolved
src/dotnet-ef/Properties/Resources.resx Outdated Show resolved Hide resolved
src/ef/Commands/MigrationsCheckPendingCommand.cs Outdated Show resolved Hide resolved
src/ef/Commands/MigrationsCheckPendingCommand.cs Outdated Show resolved Hide resolved
@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

📝 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

@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

@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 justindbaur changed the title Add migrations check-pending Command Add migrations has-pending-model-changes Command Jul 11, 2023
@justindbaur
Copy link
Contributor Author

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.

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

- Add command
- Add DatabaseFacade extension method
bricelam added a commit to justindbaur/efcore that referenced this pull request Jul 18, 2023
@bricelam
Copy link
Contributor

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

@justindbaur
Copy link
Contributor Author

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to do a missing migration check in CI
3 participants