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) Fix choco install package result #103

Open
wants to merge 25 commits into
base: development
Choose a base branch
from

Conversation

esiebes
Copy link

@esiebes esiebes commented Dec 13, 2017

edit comments from pullrequest 102

Supersedes #102

Related to #61

@esiebes esiebes changed the title Feature/chocoinstallresult Fix bug #61Feature/chocoinstallresult Dec 13, 2017
@esiebes esiebes changed the title Fix bug #61Feature/chocoinstallresult Fix bug #6 choco install package result Dec 13, 2017
@esiebes
Copy link
Author

esiebes commented Dec 13, 2017

new pullrequest originally pullrequest 102.

@ferventcoder ferventcoder changed the title Fix bug #6 choco install package result (GH-6) Fix choco install package result Jun 5, 2018
@ferventcoder ferventcoder changed the title (GH-6) Fix choco install package result (GH-61) Fix choco install package result Jun 5, 2018
@pauby pauby requested review from pauby and removed request for ferventcoder and javydekoning December 12, 2018 11:08
@@ -1,4 +1,4 @@
# Copyright (c) 2017 Chocolatey Software, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this change from the commit?

@@ -240,13 +240,9 @@ function InstallPackage
$chocoinstallparams += " $cParams"
}
Write-Verbose -Message "Install command: 'choco install $pName $chocoinstallparams'"

$packageInstallOuput = Invoke-Expression -Command "choco install $pName $chocoinstallparams"
$packageInstallOuput = Invoke-ChocoLatey "install $pName $chocoinstallparams"
Copy link
Member

Choose a reason for hiding this comment

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

Picky but can you change Invoke-ChocoLatey to a lowercase 'l'?
Can you also add the arguments parameter explicitly?

Write-Verbose -Message "Package output $packageInstallOuput "

# Clear Package Cache
Get-ChocoInstalledPackage 'Purge'
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 this has been removed?

Copy link
Author

Choose a reason for hiding this comment

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

the purge command can cause errors and hangs code in DSC configurations.

Copy link
Member

@pauby pauby Oct 7, 2019

Choose a reason for hiding this comment

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

There are no issues submitted around the purge command. If this isn't the fix for the issue itself can you raise an issue we can look at and remove it from this PR?

}

Write-Verbose -Message "Package uninstall output $packageUninstallOuput "

# Clear Package Cache
Get-ChocoInstalledPackage 'Purge'
Copy link
Member

Choose a reason for hiding this comment

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

Same here?

Copy link
Author

Choose a reason for hiding this comment

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

he purge command can cause errors and hangs code in DSC configurations.

Copy link
Member

Choose a reason for hiding this comment

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

See previous comment.

Copy link
Author

Choose a reason for hiding this comment

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

I added the purge command again.

@@ -389,41 +383,81 @@ Function Upgrade-Package {
if ($cParams) {
$chocoupgradeparams += " $cParams"
}
$cmd = "choco upgrade $pName $chocoupgradeparams"
$cmd = "upgrade $pName $chocoupgradeparams"
Write-Verbose -Message "Upgrade command: '$cmd'"
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to keep that Verbose message consistent from what it was previously - prepend it with 'choco'?

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

I don't see this change in the latest commit.

Copy link
Author

Choose a reason for hiding this comment

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

Removed the choco

# Clear Package Cache
Get-ChocoInstalledPackage 'Purge'
function Get-ChocoInstalledPackage {
Return (choco list -lo -r | ConvertFrom-Csv -Header 'Name', 'Version' -Delimiter "|")
}
Copy link
Member

Choose a reason for hiding this comment

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

What's your thinking behind running this every time rather than caching it for 60 seconds?
The Return isn't needed.

Copy link
Author

Choose a reason for hiding this comment

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

In some instances of DSC configuratioin 60 seconds is to long if a small package is installed, so the test will fail I will remove the Return

Copy link
Member

Choose a reason for hiding this comment

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

See previous comment around this.

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

if($exitcode -in $validExitCodes )
Copy link
Member

Choose a reason for hiding this comment

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

What happens if a non-MSI exits with one of the MSI exit codes? That could indicate an error but it's ignored?

Copy link
Author

Choose a reason for hiding this comment

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

its ignore/ non error

Copy link
Member

Choose a reason for hiding this comment

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

If an EXE exits with an error code that matches one of those we need that to come through. Ignoring it would indicate it installed it okay when in fact it didn't.

@pauby
Copy link
Member

pauby commented Jul 28, 2019

@esiebes Is this still a PR you are wanting to work on?

@esiebes
Copy link
Author

esiebes commented Jul 29, 2019 via email

@pauby
Copy link
Member

pauby commented Oct 3, 2019

@esiebes Are you still working on this?

@@ -325,7 +324,7 @@ Function Test-LatestVersionInstalled {

Write-Verbose -Message "Testing if $pName can be upgraded: 'choco upgrade $pName $chocoupgradeparams'"

$packageUpgradeOuput = Invoke-Chocolatey "upgrade $pName $chocoupgradeparams"
$packageUpgradeOuput = Invoke-Chocolatey "choco upgrade $pName $chocoupgradeparams"
Copy link
Member

Choose a reason for hiding this comment

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

The choco command shouldn't be passed to Invoke-Chocolatey.

@pauby
Copy link
Member

pauby commented May 21, 2020

@esiebes Are you still wanting to take this forward?

@esiebes
Copy link
Author

esiebes commented May 21, 2020 via email

@esiebes
Copy link
Author

esiebes commented May 25, 2020

I keep getting a failed test in appveyor on the psscriptanalyzer test. Local on my machine i dont get it. Someon an idea?
[-] passes the PSScriptAnalyzer Rule PSShouldProcess 986ms
Expected 0, but got 1.
[-] passes the PSScriptAnalyzer Rule PSShouldProcess 986ms
Expected 0, but got 1.
34: (Invoke-ScriptAnalyzer -Path $module.FullName -IncludeRule $rule.RuleName ).Count | Should Be 0
at , C:\projects\cchoco\tests\cChoco_ScriptAnalyzerTests.ps1: line 3434: (Invoke-ScriptAnalyzer -Path $module.FullName -IncludeRule $rule.RuleName ).Count | Should Be 0
at , C:\projects\cchoco\tests\cChoco_ScriptAnalyzerTests.ps1: line 34

@pauby
Copy link
Member

pauby commented Jun 23, 2020

Are you running the same version of PSScriptAnalzyer?

Can I also ask you to update some of those commit messages and preferably squash some of them?

@esiebes
Copy link
Author

esiebes commented Jun 23, 2020 via email

@pauby
Copy link
Member

pauby commented Jun 23, 2020

See this for the maximum version that the author believes is supported.

Also see this issue #147 for the build issues.

choco exitcode
typo

missed some bracket

add missing function

else block added, gone after merge

 should process with test not used

 add shouldprocess to Invoke chocolatey

 add verbose to scriptstest

 shouldprocess thing error

no error local found with shouldprocess

mm
orck-adrouin added a commit to orck-adrouin/cChoco that referenced this pull request Jun 8, 2022
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
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.

3 participants