Skip to content

Refactor workload set and workload install/update logic #39991

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.

Sign up for GitHub

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

Merged
merged 23 commits into from
Jul 1, 2024
Merged

Refactor workload set and workload install/update logic #39991

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
513a744
Refactor workload update and install commands
dsplaisted Apr 5, 2024
a571bba
Handle different workload update / install scenarios
dsplaisted Apr 10, 2024
c10f8df
Add Aspire test
dsplaisted May 13, 2024
5c6bf09
Roll back install state changes on workload install / update failures
dsplaisted May 14, 2024
b2ed34c
Fix workload set version prerelease handling
dsplaisted May 14, 2024
80557a6
Move WorkloadSetVersionToWorkloadSetPackageVersion to WorkloadSet class
dsplaisted May 14, 2024
45d2e5b
Support loading workload sets from different feature bands
dsplaisted May 14, 2024
22452a2
Fix issue where on some machines, VM snapshots couldn't be found
dsplaisted May 21, 2024
e47104b
Fix some tests not to skip manifest updates when installing workloads
dsplaisted May 21, 2024
e2b177f
Don't try to roll back installation of workload set package that wasn…
dsplaisted May 21, 2024
d316d96
Don't update pinned workload set on install, refactor InstallingWorkl…
dsplaisted May 21, 2024
768ec44
Fix some cases of pinning / unpinning workload version in global inst…
dsplaisted May 28, 2024
42019de
Add message for workload set installation failure
dsplaisted May 28, 2024
1d87f96
Remove unused isRollback parameter
dsplaisted May 28, 2024
515544c
Resolve TODOs in WorkloadInstallCommand
dsplaisted May 30, 2024
4bb9d50
Refactor ManifestVersionUpdate
dsplaisted Jun 13, 2024
49a9b98
Fix tests
dsplaisted Jun 26, 2024
0742587
Apply code review feedback
dsplaisted Jun 26, 2024
e6e20ad
Fix tests
dsplaisted Jun 26, 2024
708f383
Add tests for --skip-manifest-update, switch to robocopy for VM direc…
dsplaisted Jun 27, 2024
904eb64
Fix issue garbage collecting cross-feature band workload set, add err…
dsplaisted Jun 27, 2024
62c9743
Apply code review feedback and fix test
dsplaisted Jun 28, 2024
979ac00
Fix combinations of workload install, global install state, and globa…
dsplaisted Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ public override int Execute()
{
var installStateFilePath = Path.Combine(WorkloadInstallType.GetInstallStateFolder(_sdkFeatureBand, _dotnetPath), "default.json");
if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) &&
Copy link
Contributor

@Forgind Forgind Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't there be more conditions here for other reasons to require a manifest update?

Copy link
Member

Choose a reason for hiding this comment

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

clarification: consider adding string.isnull checks for --version inputs and global.json version inputs to be complete

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping on this one

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you saying that you think we should skip manifest update if --sdk-version is specified or if there's a workload version in global.json? In those cases we do want to call the UpdateWorkloadManifests method. That method is responsible for respecting the version from --sdk-version or global.json. If a version was specified in either of those ways, then it will install those versions.

So I think the logic is correct, let me know if there's a case I'm missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should skip manifest update if --sdk-version is specified or if there's a workload version in global.json

The opposite: I think we should never skip manifest updates if either of those is specified. This if statement currently says that if rollback is not specified, and there is something pinned in the install state file, then we do skip manifest updates, which is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) &&
if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) &&
!SpecifiedWorkloadSetVersionOnCommandLine &&
!SpecifiedWorkloadSetVersionInGlobalJson &&

Copy link
Member

@marcpopMSFT marcpopMSFT Jun 28, 2024 •
edited by Forgind
Loading

Choose a reason for hiding this comment

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

The explanation as I understand it is that forgind is saying that if we already have an install state, this logic will skip workload updates even if you have a global.json or --version specified (which could be different than what is in the install state). So I think you are agreeing with each other. Daniel, I think you're implying that UpdateWorkloadManifests would end up getting called for those cases which I think is true the first time a customer does it but any subsequent times after having an install state, I think forgind may be right unless I'm missing a code path that calls it elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I think I get it now. I will try to add a test to validate the behavior and then update the logic. I'm not sure how quickly I'll be able to do that though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now

!SpecifiedWorkloadSetVersionOnCommandLine &&
!SpecifiedWorkloadSetVersionInGlobalJson &&
InstallStateContents.FromPath(installStateFilePath) is InstallStateContents installState &&
(installState.Manifests != null || installState.WorkloadVersion != null))
{
Expand Down
47 changes: 46 additions & 1 deletion src/Tests/dotnet-MsiInstallation.Tests/WorkloadSetTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ void SetupWorkloadSetInGlobalJson(out WorkloadSet originalRollback)
result.StdErr.Should().Contain(versionToUpdateTo);

AddNuGetSource(@"C:\SdkTesting\workloadsets", SdkTestingDirectory);

}

[Fact]
Expand Down Expand Up @@ -294,6 +293,52 @@ public void InstallWithVersionAndSkipManifestUpdate()
.And.HaveStdErrContaining("--sdk-version");
}

[Fact]
public void InstallWithVersionWhenPinned()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the perfect test for testing this change. Simple, elegant, and it works. It'd be nice to make it run in CI, but this should be fine.

{
InstallSdk();

AddNuGetSource(@"c:\SdkTesting\WorkloadSets");

string originalVersion = GetWorkloadVersion();
originalVersion.Should().NotBe("8.0.300-preview.0.24178.1");

VM.CreateRunCommand("dotnet", "workload", "update", "--version", "8.0.300-preview.0.24178.1")
.Execute().Should().Pass();

GetWorkloadVersion().Should().Be("8.0.300-preview.0.24178.1");

VM.CreateRunCommand("dotnet", "workload", "install", "aspire", "--version", "8.0.300-preview.0.24217.2")
.Execute().Should().Pass();

GetWorkloadVersion().Should().Be("8.0.300-preview.0.24217.2");
}

[Fact]
public void InstallWithGlobalJsonWhenPinned()
{
SetupWorkloadSetInGlobalJson(out var originalRollback);

//AddNuGetSource(@"c:\SdkTesting\WorkloadSets");

string originalVersion = GetWorkloadVersion();
originalVersion.Should().NotBe("8.0.300-preview.0.24178.1");

VM.CreateRunCommand("dotnet", "workload", "update", "--version", "8.0.300-preview.0.24178.1")
.Execute().Should().Pass();

GetWorkloadVersion().Should().Be("8.0.300-preview.0.24178.1");

VM.CreateRunCommand("dotnet", "workload", "install", "aspire")
.WithWorkingDirectory(SdkTestingDirectory)
.Execute().Should().Pass();

GetWorkloadVersion(SdkTestingDirectory).Should().Be("8.0.300-preview.0.24217.2");

GetRollback(SdkTestingDirectory).Should().NotBe(originalRollback);

}

[Fact]
public void UpdateShouldNotPinWorkloadSet()
{
Expand Down