(#2112) Add msp installer support to Install-ChocolateyInstallPackage #2113
Conversation
src/chocolatey.resources/helpers/functions/Install-ChocolateyInstallPackage.ps1
Outdated
Show resolved
Hide resolved
src/chocolatey.resources/helpers/functions/Install-ChocolateyInstallPackage.ps1
Outdated
Show resolved
Hide resolved
src/chocolatey.resources/helpers/functions/Install-ChocolateyInstallPackage.ps1
Outdated
Show resolved
Hide resolved
src/chocolatey.resources/helpers/functions/Install-ChocolateyInstallPackage.ps1
Outdated
Show resolved
Hide resolved
src/chocolatey.resources/helpers/functions/Install-ChocolateyInstallPackage.ps1
Outdated
Show resolved
Hide resolved
|
@steviecoaster are you in a position to fix up the suggestions that have been made here? |
|
@gep13 I've gone through and addressed the requested changes. |
| if ($additionalInstallArgs -match 'INSTALLDIR' -or ` | ||
| $additionalInstallArgs -match 'TARGETDIR' -or ` | ||
| $additionalInstallArgs -match 'dir\=' -or ` | ||
| $additionalInstallArgs -match '\/D\=' | ||
| $additionalInstallArgs -match 'TARGETDIR' -or ` | ||
| $additionalInstallArgs -match 'dir\=' -or ` | ||
| $additionalInstallArgs -match '\/D\=' |
There was a problem hiding this comment.
This could be done with a single -match:
if ($additionalInstallArgs -match 'INSTALLDIR|TARGETDIR|dir\=|\/D\=') { ... }There was a problem hiding this comment.
Can do. Should we go this route (I know what | means here), or should we leave it as is for readability?
There was a problem hiding this comment.
For readability we can opt for something like this instead:
$argPattern = @(
'INSTALLDIR'
'TARGETDIR'
'dir\='
'\/D\='
) -join '|'
if ($additionalInstallArgs -match $argPattern) { ... }Maybe with a short comment explaining it if you feel it can use it
There was a problem hiding this comment.
To be clear... this PR is really about adding msp support, it isn't really about changing existing code, unless it is related to that change, so I don't think we need to spend too much time thinking about this change. We can make changes to code outwith the change thing that is being worked on, but it should be done as a separate commit in the PR. This is one of the things that I updated while rebasing this PR.
|
@gep13 looks like I've got all requested changes in here now! |
gep13
force-pushed
the
GH2112
branch
2 times, most recently
from
c6b0f5c to
2f07a6c
Compare
src/chocolatey.resources/helpers/functions/Install-ChocolateyInstallPackage.ps1
Outdated
Show resolved
Hide resolved
src/chocolatey.resources/helpers/functions/Install-ChocolateyInstallPackage.ps1
Show resolved
Hide resolved
src/chocolatey.resources/helpers/functions/Install-ChocolateyInstallPackage.ps1
Outdated
Show resolved
Hide resolved
|
@steviecoaster I have taken the liberty of rebasing this PR, and re-targetting against the stable branch. I have left some comments in the PR though, as I believe that there are some changes required. @vexx32 can I get you to re-review the changes and my comments as well to make sure I am not barking up the wrong tree. Thanks |
Rather than using multiple OR matches, which isn't maintainable.
It's trivial to currently work with MSI installers with chocolately, as we natively hook into msiexec's silent installation parameters to complete the installation. Support for msp files directly is a simple addition, as it uses the same underlying msiexec installation method, with a slightly modified parameter set. Co-authored-by: Stephen Valdinger <[email protected]>
|
@steviecoaster thanks for getting this added, appreciate it! |
gep13
changed the title
Fixes #2112
Verified no additional changes were required to wire this up in
Start-ChocolateyProcessAsAdmin