Skip to content
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

feat(windows): Remove UI com online-update check 📲 💽 #10156

Merged

Conversation

rc-swag
Copy link
Contributor

@rc-swag rc-swag commented Dec 6, 2023

This is just first step in #10038
Fixes: #10119

I discovered

  • - Remove all UI calls from it and use callbacks
    Not going to use call backs at all at this stage.

  • - Keep parsing separate
    For now I have this testing with kmshell.exe -buc I will have this separated more in the next PR

  • - Consider merging TOnlineUpdateCheckParams and TUpdateCheckResponse
    This I think can be done I just have a question around old version it seems that we don't want to use the old version in the Response. Probably because it is just the "previous" version. Where as the version currently installed could be even older. We could make the Fparams of type TUpdateCheckResponse as the only other piece of data we would lose is aTRemoteUpdateCheckResult which I will not be using.

  • - Figure out how SPackageUpgradeFilename is used for upgrading packages and how it fits into the new check/download, with later install model
    This was just flat file for recording the file names of the packages in the temporary data location in the case when the elevated call was required. We now have all the information we need in the cache.json

@keymanapp-test-bot skip

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Dec 6, 2023
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Dec 6, 2023

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A17S27 milestone Dec 6, 2023
@rc-swag rc-swag changed the base branch from master to epic/windows-upgrades December 6, 2023 05:35
@keymanapp-test-bot keymanapp-test-bot bot changed the title Feat/windows/remove UI com online-update check Feat/windows/remove UI com online-update check 📲 Dec 6, 2023
@rc-swag rc-swag changed the base branch from epic/windows-upgrades to epic/windows-updates December 6, 2023 05:36
@keymanapp-test-bot keymanapp-test-bot bot changed the title Feat/windows/remove UI com online-update check 📲 Feat/windows/remove UI com online-update check 📲 💽 Dec 6, 2023
@rc-swag rc-swag self-assigned this Dec 6, 2023
@rc-swag rc-swag requested a review from mcdurdin December 6, 2023 05:44
@darcywong00 darcywong00 changed the title Feat/windows/remove UI com online-update check 📲 💽 feat(windows): Remove UI com online-update check 📲 💽 Dec 6, 2023
Just using the TUpdateCheckResponse gives us enough detail
for now as we no longer have a UI set which packages to install
@github-actions github-actions bot added common/ and removed common/ labels Dec 8, 2023
@mcdurdin mcdurdin modified the milestones: A17S27, A17S28 Dec 8, 2023
@github-actions github-actions bot added common/ and removed common/ labels Dec 11, 2023
@rc-swag rc-swag marked this pull request as ready for review December 11, 2023 02:08
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

This is looking good, a bunch of style things to cleanup, but generally pretty happy with it 😁

@github-actions github-actions bot added common/ and removed common/ labels Dec 12, 2023
@github-actions github-actions bot added common/ and removed common/ labels Dec 13, 2023
@rc-swag rc-swag requested a review from mcdurdin December 13, 2023 00:44
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 184 to 191
if not DownloadFile(Params.Packages[i].DownloadURL, Params.Packages[i].SavePath) then // I2742
begin
Params.Packages[i].Install := False; // Download failed but install other files
end
else
Inc(downloadCount);
FDownload.StartPosition := FDownload.StartPosition + Params.Packages[i].DownloadSize;
end;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not DownloadFile(Params.Packages[i].DownloadURL, Params.Packages[i].SavePath) then // I2742
begin
Params.Packages[i].Install := False; // Download failed but install other files
end
else
Inc(downloadCount);
FDownload.StartPosition := FDownload.StartPosition + Params.Packages[i].DownloadSize;
end;
if not DownloadFile(Params.Packages[i].DownloadURL, Params.Packages[i].SavePath) then // I2742
begin
Params.Packages[i].Install := False; // Download failed but install other files
end
else
Inc(downloadCount);
FDownload.StartPosition := FDownload.StartPosition + Params.Packages[i].DownloadSize;
end;

else
// Keyboard Packages
FDownload.StartPosition := 0;
for i := 0 to High(Params.Packages) do
begin
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
begin
begin

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label Dec 13, 2023
@mcdurdin mcdurdin modified the milestones: A17S28, A17S29 Dec 30, 2023
@mcdurdin mcdurdin modified the milestones: A17S29, A17S30 Jan 6, 2024
@rc-swag rc-swag merged commit 9d20067 into epic/windows-updates Jan 8, 2024
6 checks passed
@rc-swag rc-swag deleted the feat/windows/remove-ui-com-onlineupdate-check branch January 8, 2024 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(windows): Refactor OnlineUpdateCheck to remove UI coupling 💽
3 participants