[ODS-6169] Update Code Generation to .NET 8 #920
Conversation
stephenfuqua
force-pushed
the
ODS-6169
branch
from
52662a6
to
2efa291
Compare
Use newer C# syntax
Re-arrange for faster feedback on build failure
stephenfuqua
force-pushed
the
ODS-6169
branch
from
b7041df
to
70bbbbb
Compare
|
|
||
| runs-on: windows-latest | ||
|
|
||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
I don't see anything in this script that would require Windows instead of Ubuntu. Note: In the Admin API project, we publish NuGet packages to Azure Artifacts using Ubuntu.
| paths: | ||
| - Application/EdFi.Admin.DataAccess/**/* | ||
| - Application/EdFi.Admin.DataAccess.UnitTests/**/* | ||
| - Application/EdFi.Common/**/* |
There was a problem hiding this comment.
@JBrenesSimpat I didn't think of this when reviewing your ticket: we only need to run this action when files in one of these three directories have changed. Please take the same approach with the Security. DataAccess.
@semalaiappan you might want to think about doing the same thing in the main branch. In limiting the action this way, a pull request only runs the jobs that are truly required - and that means we'll get feedback on the pull request more quickly.
There was a problem hiding this comment.
a small adjustment here, the path for the unit test should be - test/EdFi.Admin.DataAccess.UnitTests/**/*
There was a problem hiding this comment.
The 6.1-patch2 tag has a bug that prevents Code Generation from compiling. This workflow would have stopped that from happening.
I created a ticket to do the same in the main branch.
| - name: Run Integration Tests | ||
| working-directory: ./Ed-Fi-ODS/Utilities/CodeGeneration | ||
| run: | | ||
| dotnet test --filter FullyQualifiedName~IntegrationTests --logger "trx;LogFileName=integration-tests.trx" --logger "console;verbosity=detailed" |
There was a problem hiding this comment.
Note that I'm logging both to file and to the console. The console logging helps in debugging if there are any failures - you don't have to download the trx file.
| SerializationInfo info, | ||
| StreamingContext context) : base(info, context) | ||
| { | ||
| } |
There was a problem hiding this comment.
This is an obsolete approach in .NET 8.
| @@ -16,6 +16,69 @@ | |||
| using Microsoft.AspNetCore.Http; | |||
| using Microsoft.AspNetCore.Mvc; | |||
|
|
|||
| namespace EdFi.Ods.Api.Services.Controllers.AcademicWeeks.EdFi.Academic_Week_Readable_Excludes_Optional_References | |||
There was a problem hiding this comment.
This new code is based on profile information added in v6.1-patch1, and it should have been added to the repo in that patch release.
| @@ -14,25 +14,14 @@ | |||
|
|
|||
| namespace EdFi.Ods.CodeGen | |||
| { | |||
| public class ApplicationRunner : IApplicationRunner | |||
| public class ApplicationRunner(ITemplateProcessor templateProcessor, IEnumerable<IAssemblyDataProvider> assemblyDataProviders) : IApplicationRunner | |||
There was a problem hiding this comment.
Using newer style class constructor.
| { | ||
| throw new ArgumentNullException(nameof(cancellationToken)); | ||
| } | ||
|
|
There was a problem hiding this comment.
CancellationToken is not nullable in .NET 8.
|
@semalaiappan I added Actions workflow updates to this:
I applied these changes to Code Generation and to any files that would have been touched in other pull requests that already merged. I ignored files that will be covered in upcoming tickets. |
All approval tests are passing on my machine: