-
Notifications
You must be signed in to change notification settings - Fork 600
Review feedback for Azure.EFCore.Npgsql integration #8407
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR applies remaining feedback for the Azure.EFCore.Npgsql integration by refactoring the authentication configuration logic and updating test dependencies. Key changes include:
- Adding missing test dependency references for Npgsql tests.
- Removing the workaround file (WorkaroundToReadProtectedField) no longer needed.
- Refactoring authentication configuration by introducing the ConfigureEntraIdAuthentication extension method and updating usage across projects with NET9_0_OR_GREATER conditional support.
Reviewed Changes
Copilot reviewed 8 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/AspireEFPostgreSqlExtensionsTests.cs | Added missing using for Npgsql test utilities. |
tests/Aspire.Azure.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/WorkaroundToReadProtectedField.cs | Removed obsolete workaround for reading protected fields. |
tests/Aspire.Azure.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/AspireAzureEFPostgreSqlExtensionsTests.cs | Added missing using for Npgsql test utilities. |
src/Components/Common/ManagedIdentityTokenCredentialHelpers.cs | Refactored to expose the ConfigureEntraIdAuthentication extension method for credential setup. |
src/Components/Common/EntityFrameworkUtils.cs | Minor formatting change introduced. |
src/Components/Aspire.Azure.Npgsql/AspireAzureNpgsqlExtensions.cs | Updated to use the new authentication extension method. |
src/Components/Aspire.Azure.Npgsql.EntityFrameworkCore.PostgreSQL/AspireAzureEFPostgreSqlExtensions.cs | Updated authentication configuration logic with conditional compilation for NET9_0_OR_GREATER. |
Files not reviewed (13)
- playground/PostgresEndToEnd/PostgresEndToEnd.ApiService/PostgresEndToEnd.ApiService.csproj: Language not supported
- playground/TestShop/CatalogDb/CatalogDb.csproj: Language not supported
- playground/TestShop/CatalogModel/CatalogModel.csproj: Language not supported
- playground/TestShop/CatalogService/CatalogService.csproj: Language not supported
- playground/bicep/BicepSample.ApiService/BicepSample.ApiService.csproj: Language not supported
- playground/cdk/CdkSample.ApiService/CdkSample.ApiService.csproj: Language not supported
- playground/publishers/Publishers.ApiService/Publishers.ApiService.csproj: Language not supported
- playground/publishers/Publishers.DbSetup/Publishers.DbSetup.csproj: Language not supported
- playground/waitfor/WaitForSandbox.ApiService/WaitForSandbox.ApiService.csproj: Language not supported
- playground/waitfor/WaitForSandbox.DbSetup/WaitForSandbox.DbSetup.csproj: Language not supported
- src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.csproj: Language not supported
- tests/Aspire.Azure.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/Aspire.Azure.Npgsql.EntityFrameworkCore.PostgreSQL.Tests.csproj: Language not supported
- tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests.csproj: Language not supported
...ents/Aspire.Azure.Npgsql.EntityFrameworkCore.PostgreSQL/AspireAzureEFPostgreSqlExtensions.cs
Show resolved
Hide resolved
@roji The test Any idea? Update: I removed the use of ConfigureDataSource for tests to work: 621bb59 |
@sebastienros can you point me to the test or minimal repro and the failure so I have more context? |
...ents/Aspire.Azure.Npgsql.EntityFrameworkCore.PostgreSQL/AspireAzureEFPostgreSqlExtensions.cs
Outdated
Show resolved
Hide resolved
621bb59
to
e52cf20
Compare
tests/Aspire.Azure.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/TokenCredentialTests.cs
Outdated
Show resolved
Hide resolved
...ameworkCore.PostgreSQL.Tests/Aspire.Azure.Npgsql.EntityFrameworkCore.PostgreSQL.Tests.csproj
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I have a few nits, but the code changes are good.
Description
Applying remaining feedback from #8354
Checklist
<remarks />
and<code />
elements on your triple slash comments?breaking-change
template):doc-idea
template):