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: updated conditional statement to -ne to provide correct verbose output (fixes #446) #447

Merged
merged 11 commits into from
Jan 28, 2019

Conversation

harnsin
Copy link
Contributor

@harnsin harnsin commented Oct 9, 2018

Pull Request (PR) description

Fixes conditional statement for verbose output specifically on Line 793 in the xPackage resource to fix bug found while debugging.

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry under the Unreleased section of the change log in the README.md.
    Entry should say what was changed, and how that affects users (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 Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

@codecov-io
Copy link

codecov-io commented Oct 9, 2018

Codecov Report

Merging #447 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #447   +/-   ##
===================================
  Coverage    72%    72%           
===================================
  Files        27     27           
  Lines      4031   4031           
  Branches      4      4           
===================================
  Hits       2922   2922           
  Misses     1105   1105           
  Partials      4      4

@harnsin harnsin closed this Oct 11, 2018
@harnsin harnsin reopened this Oct 11, 2018
@harnsin harnsin closed this Oct 12, 2018
@harnsin harnsin reopened this Oct 12, 2018
@harnsin
Copy link
Contributor Author

harnsin commented Oct 12, 2018

@johlju I have tried closing and reopening the PR in hopes of resolving the error due to the integration tests in the xArchive resource, specifically the 'MSFT_xArchive.Integration.Tests' and 'MSFT_xArchive.EndToEnd.Tests' but unable to do so. I would appreciate help with these errors as they do not pertain to the change I have made in my pull request. Thank you in advance for your help.

@harnsin harnsin closed this Oct 17, 2018
@harnsin harnsin reopened this Oct 17, 2018
Copy link
Contributor

@mhendric mhendric left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mhendric
Copy link
Contributor

mhendric commented Nov 2, 2018

I've reviewed this change. The suggested changes look good to me, and the failing Integration tests look completely unrelated to this change. Furthermore, all Integration and Unit that tests related to code in MSFT_xPackageResource are still passing. I recommend that this PR be allowed to be merged.

@stale
Copy link

stale bot commented Nov 22, 2018

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@stale stale bot added the abandoned The pull request has been abandoned. label Nov 22, 2018
@mhendric
Copy link
Contributor

Hi @harnsin . Sorry for the extremely late responses on this. The failures with continuous integration should be resolved now. Can you rebase this PR and submit again? Hopefully it will pass and we can get this checked in. Thanks!

https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#resolve-merge-conflicts

@mhendric mhendric removed the abandoned The pull request has been abandoned. label Jan 24, 2019
@PlagueHO PlagueHO added the needs review The pull request needs a code review. label Jan 25, 2019
@mhendric
Copy link
Contributor

FYI, I updated the README to move the unreleased statement to the actual Unreleased section (it was under 8.5.0.0). @PlagueHO or @johlju , can one of you give this a re-review? Thanks.

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.

:lgtm:

Reviewed 1 of 1 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@PlagueHO
Copy link
Member

Should be good to go @mhendric once the AppVeyor finishes. Great stuff - love how these are all getting knocked off!

@mhendric
Copy link
Contributor

Looks like this one may actually be failing due to this change (although I'm not yet sure how). I'm going to resolve conflicts and kick of another build. If this fails again, we'll need to troubleshoot

[00:19:41] Executing script C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsPackageCab.Integration.Tests.ps1
[00:19:53]   [-] Error occurred in test script 'C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsPackageCab.Integration.Tests.ps1' 0ms
[00:19:53]     RemoteException: fatal: unable to access 'https://github.com/PowerShell/DscResource.Tests/': Could not resolve host: github.com
[00:19:53]     at Enter-DscResourceTestEnvironment, C:\projects\xpsdesiredstateconfiguration\Tests\CommonTestHelper.psm1: line 748
[00:19:53]     at <ScriptBlock>, C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsPackageCab.Integration.Tests.ps1: line 3
[00:19:53]     at <ScriptBlock>, C:\Program Files\WindowsPowerShell\Modules\Pester\4.6.0\Pester.psm1: line 1095
[00:19:53]     at Invoke-Pester<End>, C:\Program Files\WindowsPowerShell\Modules\Pester\4.6.0\Pester.psm1: line 1121
[00:19:53]     at Invoke-AppveyorTestScriptTask, C:\projects\xpsdesiredstateconfiguration\DscResource.Tests\AppVeyor.psm1: line 792

@mhendric
Copy link
Contributor

mhendric commented Jan 27, 2019

Hmmmmm. The next CI run failed again, but in a different test. The error is similar though; it was unable to connect to GitHub for some reason. Sounds like something we may want to track in an attempt to make tests in this module for resilient. I can't see how the below error is related to this particular change, but I would really like to see a successful CI run before we check this in. Thoughts?

[00:22:55] Executing script C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsFeature.Integration.Tests.ps1
[00:22:56]   [-] Error occurred in test script 'C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsFeature.Integration.Tests.ps1' 0ms
[00:22:56]     RemoteException: fatal: unable to access 'https://github.com/PowerShell/DscResource.Tests/': OpenSSL SSL_connect: SSL_ERROR_SYSCALL in connection to github.com:443 
[00:22:56]     at Enter-DscResourceTestEnvironment, C:\projects\xpsdesiredstateconfiguration\Tests\CommonTestHelper.psm1: line 748
[00:22:56]     at <ScriptBlock>, C:\projects\xpsdesiredstateconfiguration\Tests\Integration\MSFT_xWindowsFeature.Integration.Tests.ps1: line 16
[00:22:56]     at <ScriptBlock>, C:\Program Files\WindowsPowerShell\Modules\Pester\4.6.0\Pester.psm1: line 1095
[00:22:56]     at Invoke-Pester<End>, C:\Program Files\WindowsPowerShell\Modules\Pester\4.6.0\Pester.psm1: line 1121
[00:22:56]     at Invoke-AppveyorTestScriptTask, C:\projects\xpsdesiredstateconfiguration\DscResource.Tests\AppVeyor.psm1: line 792

@mhendric
Copy link
Contributor

...hey and look at that, the tests passed this time! Better merge before it changes its mind :).

Copy link
Contributor

@mhendric mhendric 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 1 files at r4, 1 of 1 files at r8.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mhendric mhendric merged commit ecaa8ac into dsccommunity:dev Jan 28, 2019
@johlju
Copy link
Member

johlju commented Jan 28, 2019

@mhendric Must have been a temporary glitch in AppVeyor? I have not seen that error before, but glitches happens now and again (not often).

@mhendric
Copy link
Contributor

Yeah, I think it was a temporary glitch. Hopefully whatever comes out of #505 will make it less likely for that to happen in the future.

@harnsin
Copy link
Contributor Author

harnsin commented Jan 30, 2019

Appreciate the help all @mhendric @johlju @PlagueHO!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review The pull request needs a code review.
Projects
None yet
5 participants