Skip to content

Conversation

borland
Copy link
Contributor

@borland borland commented Nov 13, 2024

.NET 6 is end-of-life. We are removing references to it in our projects so we can remain on a supported platform.

OctopusClients was only using .NET 6 to run tests, the library itself was netstandard2.0

This PR removes the references to .NET 6 so we only run tests against .NET 8, relying on Microsoft's compatibility through netstandard. We have seen over the years that this is very good, removing the tests should be essentially risk-free.

Additionally, I've updated third party libraries while I'm visiting.

(LocalPackagesDir / $"Octopus.Client.{FullSemVer}.nupkg").DeleteFile();
(LocalPackagesDir / $"Octopus.Server.Client.{FullSemVer}.nupkg").DeleteFile();
});
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rider autoformatter tabbed this all in

});
{
LocalPackagesDir.CreateDirectory();
(ArtifactsDir / $"Octopus.Client.{FullSemVer}.nupkg").CopyToDirectory(LocalPackagesDir, ExistsPolicy.FileOverwrite);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actual change here

CopyFileToDirectory($"{ArtifactsDir}/Octopus.Server.Client.{FullSemVer}.nupkg", LocalPackagesDir, FileExistsPolicy.Overwrite);
});
LocalPackagesDir.CreateDirectory();
(ArtifactsDir / $"Octopus.Client.{FullSemVer}.nupkg").CopyToDirectory(LocalPackagesDir, ExistsPolicy.FileOverwrite);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actual change here

<ItemGroup>
<PackageReference Include="ILRepack" Version="2.0.18" />
<PackageReference Include="Nuke.Common" Version="7.0.6" />
<PackageReference Include="ILRepack" Version="2.0.18">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Story: I upgraded ILRepack to the latest 2.0.35, however the test which asserts things get converted to internal failed; TinyTypes were being exposed as public, so I rolled it back.

The PrivateAssets thing was added by rider as part of the package upgrade/downgrade, it doesn't have any effect here anyway

</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.DotNet.Analyzers.Compatibility" Version="0.2.12-alpha">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Microsoft.DotNet.Analyzers.Compatibility has been deprecated forever, the equivalent functionality is part of the .NET SDK since .NET 5

using Octopus.Client.Model.Endpoints;
using Octopus.Client.Repositories.Async;
using Octopus.Client.Serialization;
using Assert = NUnit.Framework.Legacy.ClassicAssert;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I foolishly upgraded NUnit, who have changed their API in v4

<PackageReference Include="NUnit" Version="4.2.2" />
<PackageReference Include="NUnit3TestAdapter" Version="4.6.0" />
<PackageReference Include="TeamCity.VSTest.TestAdapter" Version="1.0.41" />
<PackageReference Include="Assent" Version="1.9.3" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Newer versions of assent only run on .net6+ so we can't upgrade past this version

<PackageReference Include="FluentAssertions" Version="6.12.0" />
<PackageReference Include="Autofac" Version="8.1.1" />
<PackageReference Include="FluentAssertions" Version="6.12.2" />
<PackageReference Include="Nancy" Version="2.0.0" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL that the integration tests use nancy to launch a webserver which fakes out responses, then OctopusClients talks to it to prove it works. Yuck, but I don't want to take this on at the moment

Comment on lines -67 to +71
Write-Output "Microsoft (R) .NET Core SDK version $(& $env:DOTNET_EXE --version)"
Write-Output "Microsoft (R) .NET SDK version $(& $env:DOTNET_EXE --version)"

if (Test-Path env:NUKE_ENTERPRISE_TOKEN) {
& $env:DOTNET_EXE nuget remove source "nuke-enterprise" > $null
& $env:DOTNET_EXE nuget add source "https://f.feedz.io/nuke/enterprise/nuget" --name "nuke-enterprise" --username "PAT" --password $env:NUKE_ENTERPRISE_TOKEN > $null
}

Choose a reason for hiding this comment

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

I don't entirely follow the environment var setting here. Is there a particular reason why we have to update the nuget source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build.ps1/.sh/.cmd are owned by the nuke tooling; their process for upgrades is that you install the nuke dotnet tool, then run nuke :update and it puts all these files up to whatever the nuke project thinks they should be. This is the source of the changes here.

Comment on lines +517 to +521
// This test disabled: It was missing lots of violations due to using a very old version of Best.Conventional with bugs in it.
// Upon upgrading to the new version, the fixed version of Best.Conventional picks up a whole bunch of violations. Fixing them
// could cause problems, so have disabled this test.
// [Test]
// public void AllResourcePropertiesShouldHavePublicSetters()
Copy link

@nick-roscarel-octo nick-roscarel-octo Nov 13, 2024

Choose a reason for hiding this comment

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

q: Do we have a plan/SC-task to follow-up on getting this test re-enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not formally, but we have decided to bring the Clients into the monorepo which will give us an opportunity to look at these kinds of things.

Personally, the convention wasn't useful and should be deleted, it's a relic from an older time that is no longer relevant.

I'll create a shortcut card to capture this anyway so we don't forget, thanks

Copy link

@nick-roscarel-octo nick-roscarel-octo left a comment

Choose a reason for hiding this comment

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

LGTM otherwise!

@borland borland merged commit 42bc730 into master Nov 17, 2024
15 checks passed
@borland borland deleted the orion/remove-net6 branch November 17, 2024 21:44
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