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

xPackage, fixes bug preventing uninstalling software #705

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

raandree
Copy link
Contributor

@raandree raandree commented Nov 25, 2020

Pull Request (PR) description

Uninstalling a .exe or .msi hangs forever as the argument handling is incorrect. The Set function calls 'msiexec.exe' and adds the installation arguments to the call. As this does not work, the process hangs waiting for input.

This Pull Request (PR) fixes the following issues

Fixes #704.

Task list

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

@raandree
Copy link
Contributor Author

I have tested the new code with this DSC configuration:

configuration PackageTest {

    Import-DscResource -ModuleName xPSDesiredStateConfiguration
    Node localhost
    {
        foreach ($package in $packages)
        {
            xPackage $package.Name
            {
                Ensure       = 'Absent'
                Name         = $package.Name
                Path         = $package.Path
                ProductId    = ''
                Arguments    = $package.Arguments
                IgnoreReboot = $true
                #FileHash     = $package.FileHash #fixed in #703
            }
        }
    }
}

$packages = @(
    @{
        Name      = 'IrfanView 4.56 (64-bit)'
        Path      = 'C:\dsc\IrfanView 4.56 (64-bit).exe'
        Arguments = '/silent'
        FileHash  = '52A48ACF59027468C7F859B78C51196E42B47C9E65D17466FF1995F71A6CFF91'
    }
    @{
        Name     = 'Node.js'
        Path     = 'C:\dsc\Node.js.msi'
        FileHash = '8CC457D650137B468D40837CC939E8BE64313974375410FDC33CF58670ADA94B'
    }
    @{
        Name      = 'Notepad++ (32-bit x86)'
        Path      = 'C:\dsc\Notepad++ (32-bit x86).exe'
        Arguments = '/S'
        FileHash  = '4747B67F7E2CA69B3EE829FA22B01FC61F500FBA530FF928E31A3D5C62BE7FFA'
    }
    @{
        Name      = 'WinRAR 5.91 (64-bit)'
        Path      = 'C:\dsc\WinRAR 5.91 (64-bit).exe'
        Arguments = '/S'
        FileHash  = '892F7FDFDAF9FCEC6C035C8C65B280E18B7797620F4375699A2D4D41F60F794C'
    }
)

Remove-Item -Path C:\DSC\*.mof -Force
PackageTest -OutputPath C:\DSC
Start-DscConfiguration -Path C:\DSC -Wait -Verbose -Force

@PlagueHO PlagueHO self-requested a review November 26, 2020 04:36
@PlagueHO PlagueHO added the needs review The pull request needs a code review. label Nov 26, 2020
@PlagueHO
Copy link
Member

Looks like the unit tests might need to be updated to fix this.

Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @PlagueHO and @raandree)


CHANGELOG.md, line 9 at r1 (raw file):

- xPackage
  - Uninstalling software (Ensure = Absent) did not work due to improper handling of arguments. This fixes #704.

Can you use the format:

- Fixes [Issue #687](https://github.com/dsccommunity/xPSDesiredStateConfiguration/issues/687).

This is so he markdown will show a link.

@PlagueHO
Copy link
Member

PlagueHO commented Dec 1, 2020

Hi @raandree - it looks like the tests are failing on this one. Can you fix and I'll complete review?

@PlagueHO
Copy link
Member

PlagueHO commented Dec 6, 2020

@raandree - can you resolve the conflicts on this one?

@raandree raandree requested a review from PlagueHO December 6, 2020 08:50
Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

Thanks @raandree - are you able to add a unit test to cover this case?

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @raandree)


CHANGELOG.md, line 8 at r3 (raw file):

## [Unreleased]

- xPackage

I also missed something in your previous PR:

We need to add a ## Fixed header above this line. Can you add this?


CHANGELOG.md, line 10 at r3 (raw file):

- xPackage
  - Uninstalling software (Ensure = Absent) did not work due to improper handling of arguments. This fixes #704.
  - Fixed a bug not allowing using the file hash of an installer [Issue #702](https://github.com/dsccommunity/xPSDesiredStateConfiguration/issues/702).

Nitpick: Just for consistency can you add a - Fixes before the issue?


source/DSCResources/DSC_xPackageResource/DSC_xPackageResource.psm1, line 552 at r3 (raw file):

                    }
                }
                catch

Is the expectation that this will catch an exception thrown from the GetValue method call?

@PlagueHO PlagueHO added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Dec 17, 2020
Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

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

Hi @raandree - are you still working on this one?

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @raandree)

@PlagueHO PlagueHO changed the base branch from master to main December 30, 2020 18:55
@PlagueHO
Copy link
Member

Hi @raandree - what I'll do is mark this abandoned and might pick it up myself when I have time.

@PlagueHO PlagueHO added abandoned The pull request has been abandoned. help wanted The issue is up for grabs for anyone in the community. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels May 29, 2021
@codecov
Copy link

codecov bot commented May 30, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@7b50109). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 8cd4a64 differs from pull request most recent head dae9bab. Consider uploading reports for the commit dae9bab to get more accurate results
Impacted file tree graph

@@          Coverage Diff          @@
##             main   #705   +/-   ##
=====================================
  Coverage        ?    72%           
=====================================
  Files           ?     30           
  Lines           ?   4428           
  Branches        ?      0           
=====================================
  Hits            ?   3213           
  Misses          ?   1215           
  Partials        ?      0           

Copy link
Contributor Author

@raandree raandree left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @PlagueHO)


CHANGELOG.md, line 9 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you use the format:

- Fixes [Issue #687](https://github.com/dsccommunity/xPSDesiredStateConfiguration/issues/687).

This is so he markdown will show a link.

Done.


CHANGELOG.md, line 8 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I also missed something in your previous PR:

We need to add a ## Fixed header above this line. Can you add this?

Done.


CHANGELOG.md, line 10 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Nitpick: Just for consistency can you add a - Fixes before the issue?

Done, or at least I think it is.


source/DSCResources/DSC_xPackageResource/DSC_xPackageResource.psm1, line 552 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Is the expectation that this will catch an exception thrown from the GetValue method call?

I see, this should not be required. GetValue handles this internally. Will change this...

@raandree
Copy link
Contributor Author

@PlagueHO, I have done some changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned The pull request has been abandoned. help wanted The issue is up for grabs for anyone in the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in xPackage resource: Uninstalling software does not work for .exe and .msi
4 participants