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.

Sign up for GitHub

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

Jump to bottom

(GH-1038) Halt operation if there is a pending reboot already existing, or from package dependency #1742

Closed
wants to merge 4 commits into from
Closed

(GH-1038) Halt operation if there is a pending reboot already existing, or from package dependency #1742

wants to merge 4 commits into from

Conversation

gep13
Copy link
Member

@gep13 gep13 commented Mar 5, 2019 •
edited by ferventcoder
Loading

Fixes #1038
Fixes #1746

@gep13 gep13 requested a review from ferventcoder March 5, 2019 09:36
@gep13
Copy link
Member Author

gep13 commented Mar 5, 2019

This is an initial implementation of what is required, and this PR is being opened for review/comment. The current implementation results in the following:

image

when attempting to install a package, where one of it's dependencies results in a reboot request.

@gep13 gep13 changed the title Halt operation if there is a pending reboot already existing, or from package dependency (GH-1038) Halt operation if there is a pending reboot already existing, or from package dependency Mar 6, 2019
ferventcoder
ferventcoder requested changes Mar 6, 2019
View reviewed changes
Copy link
Member

@ferventcoder ferventcoder 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 better. Still some things to think about here.

@gep13
Copy link
Member Author

gep13 commented Mar 7, 2019

@ferventcoder I believe that this is ready for another review. I have one question remaining about the flow of the exceptions after some more testing this morning. But I will catch up with you regarding this later.

@gep13
Copy link
Member Author

gep13 commented Mar 8, 2019

@ferventcoder updated again based on our conversations. I believe I have caught everything that was discussed.

@gep13 gep13 marked this pull request as ready for review March 8, 2019 11:17
@gep13 gep13 force-pushed the issue-1038 branch 2 times, most recently from 1a86916 to e1a36cf Compare March 8, 2019 16:05
ferventcoder
ferventcoder requested changes Mar 8, 2019
View reviewed changes
Copy link
Member

@ferventcoder ferventcoder left a comment

Choose a reason for hiding this comment

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

Some comments here in addition to the fine tooth comb review:

  • The last sentence in your commit message is unnecessary. We are typically interested in why we've introduced this code, the motivations behind it. Not how we did the work. Please remove that.
  • I don't see where you check for package exit codes during uninstallation?

    choco/src/chocolatey/infrastructure.app/services/ChocolateyPackageService.cs

    Line 916 in f57642d

    public void handle_package_uninstall(PackageResult packageResult, ChocolateyConfiguration config)
  • Please split up the commits
    • One for pending reboot detection
    • One for package reboot requested detection
    • One or more for the global validation - also create a separate issue for that so we can point to it now and in the future. It's enough of something to warrant a completely separate issue.

@gep13
Copy link
Member Author

gep13 commented Mar 9, 2019

@ferventcoder I have raised a related issue here: #1746

@gep13
Copy link
Member Author

gep13 commented Mar 9, 2019

@ferventcoder I have broken the single commit into three separate ones, in a way that I think makes sense, but let me know if there is anything that you would like to change.

gep13 added 4 commits March 11, 2019 10:33
@gep13
(maint) Corrected white space
e17d324
@gep13
(chocolateyGH-1746) Add concept of global validation
9c0da09 
This is implemented as a validation engine. Any new validations will
need to implement the IValidation interface, and be added to the
container in the ContainerBinding class.

When running with limited output, error messages will still be shown to
the user, but warning messages will be suppressed.

Initial validation is provided which errors if attempting to halt when
a reboot is detected, but when usage of package exit codes is turned
off.  This is an error, since without package exit codes turned on,
Chocolatey is not able to detect what actually happened when a package
is installed/upgraded/uninstalled.
@gep13
(chocolateyGH-1038) Stop execution if pending reboot
bde8a9d 
When executing Chocolatey commands, it is well known that a pending
reboot requirement can prevent a succesful operation. This adds the
ability to enable detection of a pending reboot (implemented as a global
validation), which will then halt execution of the command.

This addition doesn't require that the usePackageExitCodes feature be
enabled, but rather, it inspecting the current state of the system, via
the registry to determine if a reboot is required.  These checks will
only be performed when running on the Windows Operating System.
@gep13
(chocolateyGH-1038) Stop operation on package reboot request
098cafb 
There are now parameters and a feature flag which will allow
halting/overriding the install/upgrade/uninstall of a package, when
a reboot request is returned from one of it's dependencies.

When this occurs, an ApplicationException will be thrown, along with
a specific exit code, either 350 (pending reboot discovered prior to
running), or 1604 (some work completed prior to restart request) will be
returned.  This could then be inspected to decide when a reboot should
actually be performed, before continuing with the remainder of the
installation.
@ferventcoder
Copy link
Member

ferventcoder commented Mar 11, 2019 •
edited
Loading

Lost my merge commit on the rebase from upstream/stable. This was merged into stable at 964d7d0 and will be in 0.10.12. I added two additional commits for formatting the logging and xml comments.

@gep13 gep13 deleted the issue-1038 branch September 9, 2019 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ferventcoder ferventcoder ferventcoder requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gep13 @ferventcoder