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 handling of multiple nested installers #372

Merged
merged 3 commits into from
May 16, 2023

Conversation

mdanish-kh
Copy link
Contributor

@mdanish-kh mdanish-kh commented May 11, 2023

Brief Description

  1. Allocate a new installer object for each nested installer so that changes in one nested installer object do not propagate into another
  2. As a consequence of the above change, logic needs to be added for merging nested installer files into a single installer in case of NestedInstallerType: portable. This is done as:
    • If two (or more) nested portable installers have the same architecture and hash, the only difference would be of RelativeFilePath in both of them; so we merge the two installers
  3. Display name of NestedInstallerFile for better clarity before a prompt asking for additional metadata or asking whether the package is a portable.
  4. Refactor PortableCommandAliasIfApplicable() as we expect a single NestedInstallerFiles node for each installer inside the method body now.

I've listed some test URLs/manifests in the linked issue


Microsoft Reviewers: Open in CodeFlow

@mdanish-kh mdanish-kh requested a review from a team as a code owner May 11, 2023 17:33
@mdanish-kh mdanish-kh requested review from yao-msft and ryfu-msft and removed request for a team May 11, 2023 17:33
@mdanish-kh mdanish-kh force-pushed the handleMultipleNestedInstallers branch from f7cebbc to d406c80 Compare May 13, 2023 09:37
@mdanish-kh mdanish-kh force-pushed the handleMultipleNestedInstallers branch from 0766fc0 to b6bb8db Compare May 15, 2023 10:29
Copy link
Contributor

@ryfu-msft ryfu-msft left a comment

Choose a reason for hiding this comment

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

Tested manually with the examples in the bug issue thread. Implementation looks good. Thanks a ton!
:shipit:

@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft ryfu-msft merged commit f758699 into microsoft:main May 16, 2023
@mdanish-kh mdanish-kh deleted the handleMultipleNestedInstallers branch May 16, 2023 22:56
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.

2 participants