Skip to content

(GH-95) Allow for upgrading of packages#134

Open
alphakilo45 wants to merge 7 commits into
chocolatey:developmentfrom
alphakilo45:95_upgrade_packages
Open

(GH-95) Allow for upgrading of packages#134
alphakilo45 wants to merge 7 commits into
chocolatey:developmentfrom
alphakilo45:95_upgrade_packages

Conversation

@alphakilo45

Copy link
Copy Markdown

This is related to #95 and adds a new resource parameter called UpgradeLowerVersions that causes the cChocoPackageInstall resource to call choco update instead of executing an uninstall/install when the package is already installed and on a lower version.

@pauby pauby left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure why there is a UpgradeLowerVersions parameter as I would imagine upgrading, rather than uninstalling / installing is not the desired behaviour?

Comment thread DSCResources/cChocoPackageInstall/cChocoPackageInstall.psm1 Outdated
Comment thread DSCResources/cChocoPackageInstall/cChocoPackageInstall.psm1 Outdated
Comment thread DSCResources/cChocoPackageInstall/cChocoPackageInstall.psm1
Comment thread DSCResources/cChocoPackageInstall/cChocoPackageInstall.psm1 Outdated
@alphakilo45

alphakilo45 commented May 14, 2020

Copy link
Copy Markdown
Author

I went with a UpgradeLowerVersion parameter to avoid changing how the resource works by default (uninstall/install behavior). I would have expected upgrading packages to be the default behavior as well. Also all your comments should now be addressed @pauby

@pauby

pauby commented May 21, 2020

Copy link
Copy Markdown
Member

@alphakilo45 I'm not clear again as to why we are using the parameter UpgradeLowerVersion. I mean I understand why you are using it as you outlined above, but I want to say that Uninstalling and then Installing is not the correct behaviour and it should be a straight upgrade. So that being said we should alter the behaviour? I can't think of any reason why would we not want to?

Thoughts?

@alphakilo45

Copy link
Copy Markdown
Author

@pauby Only reason I can think of is when the build/pre-release tag is the only difference in the version number and that'll still be supported. I'll get the code updated.

@pauby

pauby commented Jun 23, 2020

Copy link
Copy Markdown
Member

Those tests failed quite spectacularly! Can you rebase and re-push?

@alphakilo45 alphakilo45 force-pushed the 95_upgrade_packages branch from 1865f7e to 91e9890 Compare June 26, 2020 12:40
@alphakilo45 alphakilo45 force-pushed the 95_upgrade_packages branch from 91e9890 to dd0c3ff Compare June 26, 2020 13:03
@alphakilo45

Copy link
Copy Markdown
Author

Agreed - rebase didn't help so I need to find some time on keyboard to figure out what is causing that.

@alphakilo45

Copy link
Copy Markdown
Author

@pauby Looks like Pester released v5 recently and that is causing issues with the tests. I fixed the maxed version in AppVeyor at 4.10.1 (the last pre-5.0.0 version) and tests are now running green.

Comment on lines +576 to +588
function Get-VersionCore {
[CmdletBinding()]
param (
[string]$Version
)

[Version] $versionCore = $null
if ($Version -match '(?<Major>[1-9]\d*)(\.(?<Minor>[0-9]*))?(\.(?<Patch>[0-9]*))?(\.(?<Segment>[0-9]*))?([-+].*)?') {
$versionCore = New-Object System.Version($Matches.Major, $Matches.Minor, $Matches.Patch, $Matches.Segment)
}

$versionCore
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't compare prerelease versions so 3.0.0-beta1 and 3.0.0-beta2 are the same. So in this instance the code calling it would actually uninstall 3.0.0-beta1 and install 3.0.0-beta2 rather than upgrade.

Comment on lines +476 to +477
[Parameter(Position=4)]
[string]$pVersion

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you look at line 146 above you've got:

$chocoParams += " --version='$versionToInstall'"
Upgrade-Package -pName $Name -pParams $Params -pSource $Source -cParams $chocoParams

The version parameter is added in the calling code by adding it to -cParams parameter. So I'm unsure why we need a specific parameter here that does nothing more than add it into the Choco parameters?

Comment on lines +487 to +489
if ($pVersion) {
$chocoParams += " --version=`"$pVersion`""
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is the code I was talking about above.

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.

2 participants