-
Notifications
You must be signed in to change notification settings - Fork 124
Remove references to .NET 6 #894
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
…nytypes properly. Update other deps
(LocalPackagesDir / $"Octopus.Client.{FullSemVer}.nupkg").DeleteFile(); | ||
(LocalPackagesDir / $"Octopus.Server.Client.{FullSemVer}.nupkg").DeleteFile(); | ||
}); | ||
{ |
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.
Rider autoformatter tabbed this all in
}); | ||
{ | ||
LocalPackagesDir.CreateDirectory(); | ||
(ArtifactsDir / $"Octopus.Client.{FullSemVer}.nupkg").CopyToDirectory(LocalPackagesDir, ExistsPolicy.FileOverwrite); |
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.
Actual change here
CopyFileToDirectory($"{ArtifactsDir}/Octopus.Server.Client.{FullSemVer}.nupkg", LocalPackagesDir, FileExistsPolicy.Overwrite); | ||
}); | ||
LocalPackagesDir.CreateDirectory(); | ||
(ArtifactsDir / $"Octopus.Client.{FullSemVer}.nupkg").CopyToDirectory(LocalPackagesDir, ExistsPolicy.FileOverwrite); |
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.
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"> |
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.
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"> |
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.
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; |
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.
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" /> |
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.
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" /> |
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.
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
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 | ||
} |
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.
I don't entirely follow the environment var setting here. Is there a particular reason why we have to update the nuget source?
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.
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.
// 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() |
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.
q: Do we have a plan/SC-task to follow-up on getting this test re-enabled?
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.
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
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 otherwise!
.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.