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

Error thrown when VM resource's Ensure property equals false #67

Closed
wants to merge 2 commits into from
Closed

Error thrown when VM resource's Ensure property equals false #67

wants to merge 2 commits into from

Conversation

variableresistor
Copy link
Contributor

@variableresistor variableresistor commented Nov 1, 2016

The VM's VHD's path was being verified when the VM resource's Ensure=false. If the VM doesn't exist, it doesn't make sense to verify that its VHD's Path is valid. The VM is going to be deleted anyway.


This change is Reviewable

The VM's VHD's path was being verified when the VM resource's Ensure=false. If the VM doesn't exist, it doesn't make sense to verify that its VHD's Path is valid. The VM is going to be deleted anyway.
@iainbrighton
Copy link
Contributor

Good catch! Any chance you can add a test for this so we don't get any regressions? If you need any help with this - let me know 😃


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@variableresistor
Copy link
Contributor Author

Thanks @iainbrighton 😃 I've written Pester tests before, but I'm unfamiliar with "InModuleScope" and "Mock" and I didn't want to deviate from you guys' current methodology. I know it's in the Pester documentation, so I'll have to take a look at it when I get a chance. I'll hit you up if I have any questions, if that's alright.

@kwirkykat kwirkykat added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Nov 3, 2016
Added test for: Error thrown when VM resource's Ensure property equals false #67
It verifies that the Test-TargetResource returns true if the state is "Absent", the VM is missing, and the VHD doesn't exist. If the VM is missing and we wanted it to be so, we don't care if the VHD exists.
@variableresistor
Copy link
Contributor Author

@iainbrighton , I think I've created the test correctly... It's in a separate pull request, is that okay?

@iainbrighton
Copy link
Contributor

Yes - once you fix the tabs, it looks good to me :lgtm:


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Tests/MSFT_xVMHyper-V/xVMHyper-V.Tests.ps1, line 159 at r2 (raw file):

            
            It 'Returns $true when VM is not present, "Ensure" = "Absent", and the VHD file path does not exist' {
				$targetVHDPath = 'TestDrive:\TestVHDNotExist.vhdx'

You'll need to get rid of these tabs as they'll fail the style tests.


Comments from Reviewable

@bgelens bgelens added abandoned The pull request has been abandoned. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Jun 11, 2017
@bgelens
Copy link
Contributor

bgelens commented Jun 11, 2017

Assigning abandoned label for now as there has been no response since @iainbrighton last review.
@ITGuyOU Can you address the comments?

And if you do, please also update the Readme Unreleased section with this fix

@dsccommunity dsccommunity deleted a comment from msftclas Sep 26, 2017
bgelens added a commit to bgelens/xHyper-V that referenced this pull request Jan 9, 2019
…e Present to if present block

Redoing work from abandoned PR dsccommunity#67
@bgelens bgelens mentioned this pull request Jan 9, 2019
9 tasks
bgelens added a commit that referenced this pull request Jan 11, 2019
* moved localizedData to own file
* moved parameter validation tests which are only applicable with Ensure Present to if present block
* Redoing work from abandoned PR #67
* fixed code styling issues
* fixed StartupMemory evaluation in Test-TargetResource
* changed wrong localized data variable reference AutomaticCheckpointsUnsupportedError to correct one AutomaticCheckpointsUnsupported
* Redoing work from abandoned PR #148
* updated changelog
* fix psv5 parsing error and added verbose in Get-TargetResource
* Fixed Get throws errors when no NICs or missing NIC properties
* fix Null indexing error for Get-TargetResource when VM does not exists
* added periods to end of lines in changelog
* changed multiline comments to commentblocks
@bgelens
Copy link
Contributor

bgelens commented Jan 11, 2019

Since this PR was abandoned, I redid the work in PR #163

@bgelens bgelens closed this Jan 11, 2019
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants