-
Notifications
You must be signed in to change notification settings - Fork 38
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
[ODS-6111] Update EdFi.Security.DataAccess to .Net 8 #929
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.
With this ODS/API 6.2 update to .NET 8, please keep in mind that we do not want to copy any changes from main
except for those changes that are specifically related to .NET 8.
In some circumstances we can also apply small C# improvements so long as we are 100% sure they do not change the way the code works.
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.
Please restore this file. It is still used in ODS/API 6.x code (see EdFi.Ods.Api.Container.Modules.SecurityRepositoryModule
). In ODS/API 7.x there were some recent changes to improve or remove some of the caching, which probably explains why this file would have been removed from the main
branch.
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.
@JBrenesSimpat the tests for CachedSecurityRepository
use StubSecurityContext
, which no longer compiles cleanly since they removed IDbSet
from Entity Framework Core 😠 .
There may be some other work around, but in this case we need to simply stop trying to unit test the repository. It would be ideal if we had some integration tests that access the database.
The unit tests are kind of interesting but there is not a lot of business logic to test here. We should delete the unit test file and StubSecurityContext
.
Application/EdFi.Security.DataAccess/Repositories/ISecurityRepository.cs
Outdated
Show resolved
Hide resolved
Application/EdFi.Security.DataAccess/Repositories/ISecurityRepository.cs
Outdated
Show resolved
Hide resolved
Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs
Outdated
Show resolved
Hide resolved
Application/EdFi.Security.DataAccess/Repositories/SecurityRepository.cs
Outdated
Show resolved
Hide resolved
tests/EdFi.Security.DataAccess.UnitTests/Repositories/CachedSecurityRepositoryTests.cs
Outdated
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.
@JBrenesSimpat the tests for CachedSecurityRepository
use StubSecurityContext
, which no longer compiles cleanly since they removed IDbSet
from Entity Framework Core 😠 .
There may be some other work around, but in this case we need to simply stop trying to unit test the repository. It would be ideal if we had some integration tests that access the database.
The unit tests are kind of interesting but there is not a lot of business logic to test here. We should delete the unit test file and StubSecurityContext
.
No description provided.