Skip to content

Conversation

dkurepa
Copy link
Member

@dkurepa dkurepa commented Oct 16, 2025

@Copilot Copilot AI review requested due to automatic review settings October 16, 2025 15:31
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.

Pull Request Overview

This PR migrates all projects in the arcade-services repository from targeting .NET 6.0 to .NET 8.0, removing obsolete serialization constructors and updating package versions accordingly.

Key Changes:

  • Updated TargetFramework properties from net6.0 to net8.0 (or removed to use default)
  • Removed obsolete serialization constructors from exception classes
  • Updated global.json to use .NET 8.0.415 SDK and runtime
  • Upgraded package versions to be compatible with .NET 8.0

Reviewed Changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
global.json Updated SDK version to 8.0.415 and runtime versions to 8.0.21
Directory.Packages.props Updated package versions for .NET 8 compatibility including AspNetCore, CodeAnalysis, and SqlClient packages
NuGet.config Added Microsoft.SqlServer.Server package pattern
ProductConstructionService.Common.csproj Removed multi-targeting (net6.0/net8.0) to use default .NET 8
ProductConstructionService.Api.csproj Removed version override for AspNetCore.Mvc.NewtonsoftJson
ProductConstructionService.Client.csproj Removed explicit net6.0 target framework
AuthenticationException.cs Removed obsolete serialization constructor
Microsoft.DotNet.DarcLib.csproj Removed net6.0 target and version override for System.IO.Hashing
StringUtils.cs Removed unused imports
Multiple exception files Removed obsolete serialization constructors from various exception classes
Darc.csproj Removed net6.0 target and System.IO.Hashing version pin
Maestro.MergePolicyEvaluation.csproj Removed net6.0 target framework
Maestro.MergePolicies.csproj Removed net6.0 target and System.IO.Hashing version override
Maestro.DataProviders.csproj Removed multi-targeting and System.IO.Hashing version pin
Maestro.Data.csproj Removed net6.0 target and added CS8981 warning suppression
BuildAssetRegistryContext.cs Updated FromSqlRaw to FromSql with interpolated parameters
Maestro.Common.csproj Removed multi-targeting (netstandard2.0/net6.0)
Test projects Removed AspNetCore.Mvc.NewtonsoftJson version overrides and net6.0 targets

var edgeTable = dependencyEntity.GetTableName();

var edges = BuildDependencies.FromSqlRaw($@"
var edges = BuildDependencies.FromSql($@"
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

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

The migration from FromSqlRaw to FromSql with string interpolation changes the parameter handling. With FromSql, the interpolated {buildId} on line 301 will be automatically parameterized, which is correct. However, ensure this change maintains the same SQL injection protection as the original FromSqlRaw with explicit SqlParameter.

Copilot uses AI. Check for mistakes.

Comment on lines +58 to +67
<PackageVersion Include="Microsoft.CodeAnalysis.Common" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Features" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Scripting" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic.Features" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic.Workspaces" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.Workspaces.Common" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" Version="4.5.0" />
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

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

[nitpick] The Microsoft.CodeAnalysis packages are being updated from 4.1.0 to 4.5.0. This is a relatively old version for .NET 8.0. Consider updating to a more recent version (e.g., 4.8.0 or newer) to ensure better compatibility with .NET 8 SDK features and bug fixes.

Suggested change
<PackageVersion Include="Microsoft.CodeAnalysis.Common" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Features" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Scripting" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic.Features" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic.Workspaces" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.Workspaces.Common" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" Version="4.5.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.Common" Version="4.8.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Features" Version="4.8.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Scripting" Version="4.8.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.8.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.8.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic.Features" Version="4.8.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic.Workspaces" Version="4.8.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.VisualBasic" Version="4.8.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.Workspaces.Common" Version="4.8.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" Version="4.8.0" />

Copilot uses AI. Check for mistakes.

Copy link
Member

@premun premun left a comment

Choose a reason for hiding this comment

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

So we wait with merging until we fixate the version in the 6.0 Arcade?

@dkurepa
Copy link
Member Author

dkurepa commented Oct 20, 2025

So we wait with merging until we fixate the version in the 6.0 Arcade?

I think so, I asked on the servicing chat if I can still make commits to public 6.0 repos since it's out of support, but no one responded. Will check with Matt today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants