-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support custom installer success exit codes #778
Conversation
@@ -205,7 +206,7 @@ namespace AppInstaller::CLI::Workflow | |||
context.Reporter.Warn() << "Installation abandoned" << std::endl; | |||
AICLI_TERMINATE_CONTEXT(E_ABORT); | |||
} | |||
else if (installResult.value() != 0) | |||
else if (installResult.value() != 0 && (std::find(successCodes.begin(), successCodes.end(), static_cast<int>(installResult.value())) == successCodes.end())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
installResult.value() != 0 [](start = 17, length = 26)
Should 0 be included by default if codes are provided? What if 0 is a failure for some installer? I can understand the confusion that this could create, so it might be better to have a separate flag in the manifest for making 0 a failure value. #Resolved
@@ -205,7 +206,7 @@ namespace AppInstaller::CLI::Workflow | |||
context.Reporter.Warn() << "Installation abandoned" << std::endl; | |||
AICLI_TERMINATE_CONTEXT(E_ABORT); | |||
} | |||
else if (installResult.value() != 0) | |||
else if (installResult.value() != 0 && (std::find(successCodes.begin(), successCodes.end(), static_cast<int>(installResult.value())) == successCodes.end())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int [](start = 112, length = 3)
Should we just store the values in the manifest as DWORD then, since we will always compare against this value type? #Resolved
@@ -205,7 +206,7 @@ namespace AppInstaller::CLI::Workflow | |||
context.Reporter.Warn() << "Installation abandoned" << std::endl; | |||
AICLI_TERMINATE_CONTEXT(E_ABORT); | |||
} | |||
else if (installResult.value() != 0) | |||
else if (installResult.value() != 0 && (std::find(successCodes.begin(), successCodes.end(), static_cast<int>(installResult.value())) == successCodes.end())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
successCodes [](start = 58, length = 12)
I think we are making a problem for ourselves later if we simply add this support without a healthy set of defaults in place. If we take this as is, this is the problem I imagine:
- Installer always return some value stating "reboot required for full functionality"
- Manifest author puts this value in success codes
- We add defaults later that make this value and others success for installation
- Installer starts returning one of the other values in some cases (maybe reboot required for any functionality)
- We fail the install because it isn't one of the values in the list
Then do we need to start treating the values in the manifest as additional ones, beyond 0 and our defaults? How could they indicate to us that our defaults are incorrect; that one of them means a failure?
If the manifest called these AdditionalSuccessCodes
, we would probably be fine to treat 0 and any values we wanted (on a per-InstallerType basis) as success. But if it calls them InstallerSuccessCodes
, it seems like this is the list, full stop. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added tests for custom installer success codes.
Microsoft Reviewers: Open in CodeFlow