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

Add expected installer return codes #1421

Merged
merged 21 commits into from
Sep 15, 2021

Conversation

florelis
Copy link
Member

@florelis florelis commented Aug 31, 2021

Adding expected return codes for installer. Expected return codes are translated to specific HResults when found and show a custom error message.

  • Updated v1.1 manifest schema with new field for expected return codes. Updated file would look like this:
Installers: 
    - InstallerUrl: https://example.com
      InstallerType: exe
      ExpectedReturnCodes:
          PackageInUse: 1
          InstallInProgress: 2
  • Updated manifest parsing and validation for the new fields.
  • Refactored common code for checking installer exit code from MSI and ShellExecute installs into a workflow task that now also checks the expected return codes.

Not in this PR: Changes for REST sources

Microsoft Reviewers: Open in CodeFlow

@florelis florelis requested a review from a team as a code owner August 31, 2021 01:43
Copy link
Member

@JohnMcPMS JohnMcPMS left a comment

Choose a reason for hiding this comment

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

We need to make sure we have the full set of cases covered; "reboot required" being missing suggests that we have some to get still.

Also, the arguably more important part of this change is having a healthy set of default values per installer type and thorough thought put into how those would be overridden by the manifest. Requiring every MSI to output the fairly standardized set of values is not helpful.

This also plays into my comment on MSIX, as we can normalize the HRESULT returns from deployment into the set of values that we have here to make all package types easier to work with in a uniform fashion.

src/AppInstallerCommonCore/Public/AppInstallerErrors.h Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Workflows/InstallFlow.cpp Outdated Show resolved Hide resolved
@florelis
Copy link
Member Author

Updated with default values per installer type.
I changed the way this is represented in the manifest because I hadn't considered multiple return codes mapping to the same response. The format is now

      ExpectedReturnCodes:
          - InstallerReturnCode: 1
            ReturnResponse: PackageInUse

@github-actions

This comment has been minimized.

Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@florelis florelis merged commit 2a9d1c6 into microsoft:master Sep 15, 2021
@florelis florelis deleted the ExpectedReturnCode branch September 27, 2021 20:10
@denelon denelon linked an issue Oct 1, 2021 that may be closed by this pull request
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.

Expected return codes
4 participants