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

Spike WithEntityFrameworkMigration #7894

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mitchdenny
Copy link
Member

DO NOT MERGE

This explores the idea of adding a WithEntityFrameworkMigrations(...) extension method which creates an executable that calls out to dotnet ef database update. At the moment this code doesn't work because we are being impacted by the dreaded:

/workspaces/aspire/playground/waitfor/WaitForSandbox.ApiService/WaitForSandbox.ApiService.csproj : error MSB4057: The target "GetEFProjectMetadata" does not exist in the project.
Unable to retrieve project metadata. Ensure it's an SDK-style project. If you're using a custom BaseIntermediateOutputPath or MSBuildProjectExtensionsPath values, Use the --msbuildprojectextensionspath option.

But conceptually dynamically adding an executable and passing along the connection string works.

@Copilot Copilot bot review requested due to automatic review settings March 5, 2025 00:11
@mitchdenny mitchdenny self-assigned this Mar 5, 2025
@mitchdenny mitchdenny added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Mar 5, 2025 — with GitHub Codespaces
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR explores adding an extension method, WithEntityFrameworkMigrations, to streamline running Entity Framework migrations by generating a dedicated executable that calls "dotnet ef database update". Key changes include:

  • Adding the WithEntityFrameworkMigrations extension method and a corresponding marker interface.
  • Updating project initialization in playground files to integrate the new migration mechanism.
  • Adjusting namespaces and removing an obsolete DbSetup program in favor of the new approach.

Reviewed Changes

File Description
src/Aspire.Hosting/EntityFrameworkMigrationExtensions.cs New extension method for EF migrations with executable creation.
src/Aspire.Hosting/ApplicationModel/IResourceSupportsEntityFrameworkMigrations.cs New marker interface enabling migration support.
playground/waitfor/WaitForSandbox.AppHost/Program.cs Updated to invoke the new migration method.
playground/waitfor/WaitForSandbox.ApiService/Model/Entry.cs.cs Namespace update from Common to ApiService.Model.
playground/waitfor/WaitForSandbox.ApiService/Program.cs Namespace update to reflect new model namespace.
playground/waitfor/WaitForSandbox.ApiService/Model/MyDbContext.cs Namespace update to reflect new model namespace.
src/Aspire.Hosting.Azure.PostgreSQL/AzurePostgresFlexibleServerDatabaseResource.cs Extended resource to support EF migrations.
playground/waitfor/WaitForSandbox.DbSetup/Program.cs Removed obsolete DbSetup program.

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

context.Args.Add("--project");
context.Args.Add(projectMetadata.ProjectPath);
context.Args.Add("--connection");
context.Args.Add(builder);
Copy link
Preview

Copilot AI Mar 5, 2025

Choose a reason for hiding this comment

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

The argument passed for '--connection' is 'builder', which is not a connection string. Consider passing the actual connection string value obtained from the resource instead.

Suggested change
context.Args.Add(builder);
context.Args.Add(builder.GetConnectionString());

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@mitchdenny
Copy link
Member Author

Ergonomics wise it might make sense to have a WithMigrations(...) extension method which is hand-crafted for each resource type for which we support migrations instead of what I've done here which is trying to make it generic at the Aspire.Hosting package level.

This would be better from an ergonomics point of view because we'd be able to do this:

builder.AddPostgres(...).AddDatabase(...).WithEntityFrameworkMigration<Projects.ProjectWithDbContext>();

It looks like the MSBuild issues with dotnet ef database update and dotnet ef migrations add have been solved in .NET 10 - but there is no plan to backport them. So maybe this isn't something we can do before we start on the .NET Aspire 10 round of enhancements?

@DamianEdwards DamianEdwards marked this pull request as draft March 5, 2025 00:16
@DamianEdwards
Copy link
Member

It looks like the MSBuild issues with dotnet ef database update and dotnet ef migrations add have been solved in .NET 10 - but there is no plan to backport them. So maybe this isn't something we can do before we start on the .NET Aspire 10 round of enhancements?

We should figure out the details and push to backport if we think it's needed for this scenario.

context.Args.Add("update");
context.Args.Add("--project");
context.Args.Add(projectMetadata.ProjectPath);
context.Args.Add("--connection");
Copy link
Member

Choose a reason for hiding this comment

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

you need to pass the --no-build flag too so it doesn't try to build the project while it's running

@afscrome
Copy link
Contributor

afscrome commented Mar 5, 2025

I have been doing something very similar to this successfully for a while now:

var migrations = builder.AddExecutable("dbMigrations", "dotnet", "../My.Migration.Project")
   .WithArgs(
      "ef",
      "database",
      "update",
      "--connection",
      database.Resource.ConnectionStringExpression,
      "--startup-project",
      "../My.Api",
      "-v",
      "--no-build") // As this project is part of the solution, relying on the fact that it has just been built before running the app host
   .WaitFor(postgres)
   .WithParentRelationship(database)
   ;

builder.AddProject<My_Api>("api")
   .WaitFor(migrations)
   .WithEnvironment("MY_ConnectionString", database.Resource.ConnectionStringExpression)
   ;

context.Args.Add(builder);
})
.WaitFor((IResourceBuilder<IResource>)builder);
return builder;
Copy link
Contributor

@afscrome afscrome Mar 5, 2025

Choose a reason for hiding this comment

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

What if your app wants to wait for migrations to be completed? With thisapproach, your app would need to wait on the migrator resource, but the migrator resource is not returned to the consumer?

As an alternative to running dotnet ef migrate as a separate process, you could run the migrations in-process via the ResourceReadyEvent - as the event is blocking, consumers could just wait on the database resource directly, although it would require a bit more code to write, wire up logs, handle errors etc.

Copy link
Member Author

@mitchdenny mitchdenny Mar 5, 2025

Choose a reason for hiding this comment

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

We are considering this option as well. The downside of that is that you need to structure your project a certain way. It's all tradeoffs.

Copy link
Member

Choose a reason for hiding this comment

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

Running it in process mean referencing the DbContext and all of the app code from the apphost, which is a non starter.

context.Args.Add("--connection");
context.Args.Add(builder);
})
.WaitFor((IResourceBuilder<IResource>)builder);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does dotnet ef migrate emit open telemetry?

Suggested change
.WaitFor((IResourceBuilder<IResource>)builder);
.WaitFor((IResourceBuilder<IResource>)builder)
.WithOtlpExporter()
;

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a good question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants