Skip to content

Workload manifest updater cleanup #35859

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

Jump to bottom
Merged
merged 11 commits into from
Oct 12, 2023
Merged

Workload manifest updater cleanup #35859

merged 11 commits into from
Oct 12, 2023

Conversation

MiYanni
Copy link
Member

@MiYanni MiYanni commented Oct 4, 2023

Related: #22882

Summary

I was planning on working on the linked issue above, but ended up going down a rabbit hole. A majority of the changes take place in WorkloadManifestUpdater.cs. A lot of the cleanup is around the uses of many different ValueTuple types. I created records or aliases to attempt to simplify the situation. No functional changes should be taking place in this PR.

Notable Changes

  • Created the ManifestUpdateWithWorkloads and ManifestVersionWithBand records to help with some of the tuple situation.
  • Removed _tempDirPath as it was unused. The value is currently part the WorkloadInstallerFactory.GetWorkloadInstaller(), so the provided IWorkloadManifestInstaller will contain and use this temp directory.
    • Similarly, _tempDirManifestPath was not used in the mocked version for testing, so this was removed too.
  • Created a WorkloadCollection alias since a dictionary of WorkloadId to WorkloadDefinition is used in multiple places in the code.
  • Used deconstruction in many places to simplify readability.
  • Refactored GetManifestPackageDownloadsAsync into a loop since it was a series of if-statements that did the same logic 3 times in a row.
  • Ad-hoc formatting cleanup and some style simplification.

@MiYanni MiYanni requested a review from a team October 4, 2023 23:37
@ghost ghost added Area-Workloads untriaged Request triage from a team member labels Oct 4, 2023
@dsplaisted
Copy link
Member

I haven't looked at the code changes yet, but this should probably target 8.0.2xx instead of main, since we're going to continue working on workloads in 8.0.2xx, and if we put a refactoring like this in main it will be harder to merge later.

nagilson
nagilson reviewed Oct 5, 2023
View reviewed changes
Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

Some suggestions for this change, overall I am quite happy with these improvements, so thank you :)

@MiYanni
Copy link
Member Author

MiYanni commented Oct 6, 2023

@dsplaisted

but this should probably target 8.0.2xx instead of main

🤔 The situation is:

  1. I did this change previously which is breaking change to the workload commands that output JSON. This was done in main.
  2. This current PR is a precursor for this issue which is a follow-up for the PR I just mentioned. So, the changes for that issue should definitely be main as it is modifying the JSON output (even though the proposed changes are additive).

So, is it a problem if we merge this into 8.0.2xx and then (using /backport) also put it into main? Or should I just wait until the codeflow happens for 8.0.2xx -> main? I'd like this in main soon to do my next set of changes.

Forgind
Forgind approved these changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

A few minor things, but overall, this looks great! Thanks for making this change!

MiYanni added 9 commits October 9, 2023 16:03
@MiYanni
A lot of cleanup around uses of many different ValueTuple types. Crea…
14fa66f 
…ted records or aliases to attempt to simplify. Ad-hoc formatting cleanup and some simplification. No functional changes should be taking place.
@MiYanni
Refactored GetManifestPackageDownloadsAsync to be a loop. Cleaned up …
eba98e2 
…a few other minor things.
@MiYanni
Removed extra empty line.
a6311ae
@MiYanni
Modified GetManifestPackageDownloadsAsync to use a foreach with Disti…
6fb2d3a 
…nct to eliminate duplicates.
@MiYanni
Cleaned up GetManifestPackageUpdate.
4315a53
@MiYanni
Moved a call in CalculateManifestUpdates to be closer to its usage.
98a7521
@MiYanni
Removed unused statements in the GivenNoWorkloadsInstalledInfoOptionR…
6f54c4b 
…emarksOnThat test.
@MiYanni
Removed string.Format use for reporter.WriteLine since it has an inte…
919c985 
…rface that does this automatically.
@MiYanni
Fix build issue after rebase to feature/8.0.2xx.
64f8fe6
@MiYanni MiYanni force-pushed the VariousWorkloadCodeCleanup branch from 3499958 to 64f8fe6 Compare October 9, 2023 23:21
@MiYanni MiYanni requested review from a team and andriipatsula as code owners October 9, 2023 23:21
@MiYanni MiYanni changed the base branch from main to release/8.0.2xx October 9, 2023 23:21
@MiYanni
Added code comments to GetManifestPackageDownloadsAsync. Updated Upda…
b402558 
…teAdvertisingManifestAsync to match the same pattern as GetManifestPackageDownloadsAsync, since it was using the same previous pattern except with 2 bands instead of 3.
nagilson
nagilson approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

LGTM now, this is a large improvement, thank you for making this :)

@MiYanni MiYanni merged commit 1e6e115 into dotnet:release/8.0.2xx Oct 12, 2023
@MiYanni MiYanni deleted the VariousWorkloadCodeCleanup branch October 12, 2023 20:17
@dsplaisted
Copy link
Member

This looks good, thanks!

Could / should we use a global using for WorkloadCollection so it doesn't need to be repeated in different files?

@MiYanni
Copy link
Member Author

MiYanni commented Oct 12, 2023 •
edited
Loading

@dsplaisted

Could / should we use a global using for WorkloadCollection so it doesn't need to be repeated in different files?

Oh, that's a great idea! I forgot you could put aliases in those. I'll do that in a separate PR real quick.

Edit: New PR here: #36092

@MiYanni MiYanni mentioned this pull request Oct 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Forgind Forgind Forgind approved these changes

@nagilson nagilson nagilson approved these changes

@andriipatsula andriipatsula Awaiting requested review from andriipatsula

@dsplaisted dsplaisted Awaiting requested review from dsplaisted

Assignees
No one assigned
Labels
Area-Workloads untriaged Request triage from a team member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MiYanni @dsplaisted @Forgind @nagilson