Skip to content

Support installing workload pack groups#24630

Merged
dsplaisted merged 13 commits into
dotnet:release/6.0.3xxfrom
dsplaisted:bundle-workload-packs
Apr 1, 2022
Merged

Support installing workload pack groups#24630
dsplaisted merged 13 commits into
dotnet:release/6.0.3xxfrom
dsplaisted:bundle-workload-packs

Conversation

@dsplaisted

@dsplaisted dsplaisted commented Mar 30, 2022

Copy link
Copy Markdown
Member
  • Supports installing workload pack group MSIs (Support grouping multiple workload packs in a single MSI #21741). Creating the MSIs will be a separate update to the workload creation infrastructure in dotnet/arcade.
  • Creates a ManifestVersionUpdate class to replace passing around tuples of 5 elements in a lot of places
  • Switches to a new transaction infrastructure for workloads, to allow for more control of the ordering of messages and rollback actions, as well as making it explicit which installation methods should participate in the transaction (by taking an ITransactionContext parameter)

(This is rebased on top of #24545, so it also currently includes commits from that PR.)

@ghost

ghost commented Mar 30, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@dsplaisted dsplaisted requested review from a team, gkulin and joeloff March 30, 2022 14:31
var manifests = _workloadResolver.GetInstalledManifests();
foreach (var manifest in manifests)
{
var packGroupFile = Path.Combine(manifest.ManifestDirectory, "WorkloadPackGroups.json");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the new mapping file we're going to add to the manifest installers?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct.

public int CompareTo(ManifestVersionUpdate? other)
{
if (other == null) return 1;
int ret = ManifestId.CompareTo(other.ManifestId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For a version update of a manifest, shouldn't the IDs always be the same and the underlying versions be different. Should we instead throw something like InvalidOperationException if the IDs are different?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is just implemented so that equality works correctly, so that in tests you can assert that the manifest updates were what was expected.

@joeloff joeloff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I'm good with this. The tricky bit is going to be computing the group IDs/names, but that needs to happen in Arcade

This can be a workaround if there are bugs in the pack groups or the installation code
@dsplaisted dsplaisted marked this pull request as ready for review March 31, 2022 23:19
@dsplaisted dsplaisted merged commit 9dc5b2e into dotnet:release/6.0.3xx Apr 1, 2022
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.

3 participants