Detect conflicting settings when using EnrichEF #3159
Conversation
dotnet-issue-labeler
bot
added
the
area-integrations
# Conflicts: # src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs
davidfowl
changed the title
src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs
Outdated
Show resolved
Hide resolved
|
Just for Npgsql for now to validate the fix. I think the way the execution strategy is checked is too brittle (passing a |
|
|
||
| if (settings.Retry) | ||
| { | ||
| var executionStrategy = extension?.ExecutionStrategyFactory?.Invoke(new ExecutionStrategyDependencies(null!, optionsBuilder.Options, null!)); |
There was a problem hiding this comment.
@sebastienros just to confirm, you're referring to this right?
/cc @AndriySvyryd
There was a problem hiding this comment.
Yes, new ExecutionStrategyDependencies(null! I don't know any other way to get the actual ExtensionStrategy, and our tests are actually passing a concrete DbContext because we own the type and know how to construct it. There this code makes me sad, the ctor in other EF providers could definitely guard against it, right?
There was a problem hiding this comment.
Yes, there's no other way for 8. But, I assume all of this will be removed for the next version, right? You'd just need to make sure that the user calls Enrich before AddDbContext, so all Aspire configuration can be silently overridden.
For SqlServer you can get this behavior in 8 by calling UseAzureSqlDefaults instead of EnableRetryOnFailure
There was a problem hiding this comment.
You'd just need to make sure that the user calls Enrich before AddDbContext
I don't think that's the API experience we are looking for. IMO, the current experience makes more sense:
- AddDbContext
- EnrichDbContext
If you flip it the other way around, it doesn't really make sense. How can you "Enrich" something before you add it?
There was a problem hiding this comment.
Ok, perhaps the question is whether Enrich just provides a recommended default configuration or whether it should be authorative over anything specified in AddDbContext.
Personally, I don't see why it should override or throw for any conflicting configuration. A suppressible warning seems more appropriate.
tests/Aspire.Microsoft.EntityFrameworkCore.Cosmos.Tests/EnrichCosmosDbTests.cs
Outdated
Show resolved
Hide resolved
tests/Aspire.Microsoft.EntityFrameworkCore.SqlServer.Tests/EnrichSqlServerTests.cs
Outdated
Show resolved
Hide resolved
|
/backport to release/8.0-preview5 |
|
Started backporting to release/8.0-preview5: https://github.com/dotnet/aspire/actions/runs/8446763097 |
|
@sebastienros backporting to release/8.0-preview5 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Detect conflgicting settings when using EnrichEF
Using index info to reconstruct a base tree...
M src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs
CONFLICT (content): Merge conflict in src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/AspireEFPostgreSqlExtensions.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Detect conflgicting settings when using EnrichEF
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@sebastienros an error occurred while backporting to release/8.0-preview5, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
Fixes #2524
Microsoft Reviewers: Open in CodeFlow