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

Improve ZIP handling in interactive update #398

Merged
merged 3 commits into from
Jul 10, 2023

Conversation

mdanish-kh
Copy link
Contributor

@mdanish-kh mdanish-kh commented Jul 3, 2023

This change makes it so that the zip archive is extracted in interactive update as well and the path to the nested installer is used in other functions. Made the logic of extracting and determining path of the nested installer into a function as it's common between autonomous and interactive updates.

The logic is based off of the fact that if InstallerType is ZIP, there will always be atleast one NestedInstallerFile node in the manifest that contains RelativeFilePath key and the first matching RelativeFilePath from an older manifest is used as the path to the nested installer. Multiple RelativeFilePath nodes under NestedInstallerFiles are only possible if the NestedInstallerType is portable. In this PR, we do not check the InstallerType of all RelativeFilePath nodes for NestedInstallerType: portable. The assumption is that it's very rare that if an older version contained all portable RelativeFilePath objects, then one of them has changed in newer version to be an another InstallerType (e.g. MSI, nullsoft etc). In case of portables as well, only the first matching RelativeFilePath object is passed to determine the InstallerType to keep the logic simple.

Shifting of values from root to installer is needed in cases where the NestedInstallerFiles object is common between all installer nodes and is present in the root of the manifest instead for better readability. One of the many examples of such manifests is junnegun.fzf. Since the interactive update only works by reading the Installers node under the installer manifest, we need a way to read the NestedInstallerFiles in the root of the manifest. I did it by shifting all related values to installer level at the start of the interactive update. Ofcourse this comes at a cost of slightly more verbose manifest, but I think it's okay for now considering the benefit of a successful update. We can make another function ShiftInstallerValuesToRoot to take out common fields to the root level but I think that's beyond the scope of this PR as it's applicable to new, autonomous update commands as well and being tracked in issue #185


Microsoft Reviewers: codeflow:open?pullrequest=#398

@mdanish-kh mdanish-kh requested a review from a team as a code owner July 3, 2023 10:25
@mdanish-kh mdanish-kh requested review from yao-msft and ryfu-msft and removed request for a team July 3, 2023 10:25
@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft ryfu-msft linked an issue Jul 10, 2023 that may be closed by this pull request
@ryfu-msft ryfu-msft merged commit 6dfdb12 into microsoft:main Jul 10, 2023
@ryfu-msft
Copy link
Contributor

Apologies for the delay, since the interactive update needs to be manually tested, I needed to checkout your branch and try it manually in order to review this. Shifting the root properties to the installer node is a great addition and it will help improve the interactive flow.

Once thing I wanted to call out was that I found a bug when trying to update junnegun.fzf interactively. All of the architectures get updated to 'x64' thus making the updated manifest invalid. I created an issue here: #405 but I went ahead and approved this PR since that is not related to the feature you just added. Thanks!

@mdanish-kh mdanish-kh deleted the failedtoParseInteractively branch July 10, 2023 22:50
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.

ZIP Packages update autonomously but not in interactive mode
2 participants