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

(GH-61) Validate exit code when installing package #168

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

orck-adrouin
Copy link

@orck-adrouin orck-adrouin commented Jun 8, 2022

Description

Changes cChocoPackagesInstall to validate choco's exit code. Handles the standard exit codes as documented in Chocolatey's documentation.

This PR is based on pullrequest #103 that has not seen any updates in the last 2 years. Instead of rebasing the PR I cloned the cChoco repo and I copied over (and improved) the changes from the original author.

Related Issue

Fixes #61

Motivation and Context

It allows a DSC to report a failure when Chocolatey fails to install a package.

How Has This Been Tested?

I used the DSC that was provided in issue #61 to simulate an installation failure. Unfortunately, I was not able to find Chocolatey packages to test the different exit codes outlined in this document: https://docs.chocolatey.org/en-us/choco/commands/install#exit-codes .

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Changes cChocoPackagesInstall to validate choco's exit code. Handles the
standard exit codes as documented in Chocolatey's documentation.

Fixes issue chocolatey#61
Supersedes chocolatey#103
$p.Dispose()

#Set $LASTEXITCODE variable.
powershell.exe -NoLogo -NoProfile -Noninteractive "exit $exitcode"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to instantiate a powershell.exe instance instead of just setting $LastExitCode directly?

Copy link
Author

Choose a reason for hiding this comment

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

This PR is basically a "port" of the old PR #103 from 2017 to the latest code base so I cannot tell you exactly why they did it that way.

The only reason I can see to do it that way is to ensure that the real $LastExitCode is set. Setting $LastExitCode directly would create a variable named $LastExitCode which is scoped to the function Invoke-ChocoProcess and it would not set the global $LastExitCode variable.

You can reproduce the behavior with this code

function Test {
    $LASTEXITCODE = 3 # this will create a variable scoped to the function and will not impact its parent scope

   # you can uncomment this line to set the global $LastExitCode 
   # powershell.exe -NoLogo -NoProfile -Noninteractive "exit 3"
}

#fake a lastexitcode
powershell.exe -NoLogo -NoProfile -Noninteractive "exit 123"

write-Host "Value before Test function: $LASTEXITCODE" # will print 123 for the exit code (set by the previous line)
Test
write-Host "Value after Test function: $LASTEXITCODE" # value stays at 123 because the exit code variable in the Test function is scope to that function

@@ -161,6 +161,68 @@ Describe -Name "Testing $ResourceName loaded from $ResourceFile" -Fixture {
}
}

Context -Name "Package cannot be found" -Fixture {
Copy link
Member

Choose a reason for hiding this comment

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

All of these tests added will likely require adjustments when #163 is merged due to the upgrade to Pester 5. (Not suggesting anything needs to be done around this at this particular moment. Just to keep in mind.)

}
}

function Invoke-ChocoProcess {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason to use this method to create the Chocolatey process instead of & or Start-Process? Or even the Invoke-Expression that was originally being used?

Copy link
Author

Choose a reason for hiding this comment

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

From what I remember from 13 months ago, I had some issues getting a reliable last exit code with Invoke-Expression so I used to the code from the very old and possibly abandoned PR #103 which address the same issue.

@corbob corbob self-assigned this Jul 6, 2023
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.

cChocoPackageInstall doesn't check that the package was actually installed
2 participants