add feature for Host Power Policy Setting #17
Conversation
Integrated new resource for VMHostPowerPolicy Included Configuration file Missing Unit tests because of issue vmware#16 Signed-off-by: Markus Fischbacher <fischbacher.markus@gmail.com>
Source/VMware.vSphereDSC/Tests/Unit/VMHostPowerPolicySettings.Unit.Tests.ps1
Show resolved
Hide resolved
|
@rockaut, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
on @SimeonGerginov suggestions Signed-off-by: Markus Fischbacher <fischbacher.markus@gmail.com>
rockaut
force-pushed
the
feature-pwrpolicy
branch
from
f6ce920
to
86b8da8
Compare
Signed-off-by: Markus Fischbacher <fischbacher.markus@gmail.com>
Signed-off-by: Markus Fischbacher <fischbacher.markus@gmail.com>
Will reimplement it with enum/strings later Signed-off-by: Markus Fischbacher <fischbacher.markus@gmail.com>
Cleaned some code too. Signed-off-by: Markus Fischbacher <fischbacher.markus@gmail.com>
|
May you check again please? Regarding the enum for the power policy - i will implement that later on. |
There was a problem hiding this comment.
Good job. I have one last request - if you can update your fork, because I have extracted the resources in separate files and added build script. So when you do that run the tests again, so we can be sure that everything is alright. After that I will merge it. Once again, good job !
|
For clarification... should we run the build script prior to commit? |
|
Yes, for now run the build script prior to commit. |
|
I checked out dev branch again and getting tons of unit test errors. I merged dev into my branch and cant get the unit tests for PowerSettings working again. The errors on my class resembles more or less the errors for the current classes. |
|
For example one of the errors for VMHostNtpSettings |
|
@rockaut, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
|
@rockaut, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
|
@rockaut, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
rockaut
force-pushed
the
feature-pwrpolicy
branch
from
d7993ba
to
daf4003
Compare
|
@rockaut, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
rockaut
force-pushed
the
feature-pwrpolicy
branch
from
daf4003
to
f6612ff
Compare
|
@rockaut, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
Signed-off-by: Markus Fischbacher <fischbacher.markus@gmail.com>
rockaut
force-pushed
the
feature-pwrpolicy
branch
from
f6612ff
to
ed7df88
Compare
Signed-off-by: Markus Fischbacher <fischbacher.markus@gmail.com>
|
Are the Unit Tests still failing after the new commits ? |
Signed-off-by: Markus Fischbacher <fischbacher.markus@gmail.com>
Signed-off-by: Markus Fischbacher <fischbacher.markus@gmail.com>
|
@SimeonGerginov no, after merging with current dev now all green again. |
| } | ||
|
|
||
| Configuration VMHostPowerPolicySettings_Config | ||
| { |
There was a problem hiding this comment.
Move the Configuration and Node opening brackets on the same line for consistency with other configurations.
| Host setting for power policy | ||
| #> | ||
| [DscProperty(Mandatory)] | ||
| [System.Int32] $PowerPolicy = 2 |
There was a problem hiding this comment.
For simplicity set the type to be [int].
| [System.Int32] $PowerPolicy = 2 | ||
|
|
||
| [void] Set() | ||
| { |
There was a problem hiding this comment.
Move all opening brackets to the same line in the whole class.
| $vmHost = $this.GetVMHost() | ||
|
|
||
| $vmHostCurrentPowerPolicy = $vmHost.ExtensionData.Config.PowerSystemInfo.CurrentPolicy | ||
| $shouldUpdateVMHostPowerPolicy = $this.ShouldUpdateVMHostPowerPolicy($vmHostCurrentPowerPolicy) |
There was a problem hiding this comment.
I think the call to the helper method here is redundant, because if we got here that means that the Test() method returned false so the PowerPolicy should be updated.
| { | ||
| $this.ConnectVIServer() | ||
| $vmHost = $this.GetVMHost() | ||
| $vmHostPowerPolicy = $vmHost.ExtensionData.Config.PowerSystemInfo.CurrentPolicy |
There was a problem hiding this comment.
Rename to $vmHostCurrentPowerPolicy.
| $this.ConnectVIServer() | ||
| $vmHost = $this.GetVMHost() | ||
|
|
||
| $result.Name = $this.Name |
There was a problem hiding this comment.
The Name should be retrieved from the server: $result.Name = $vmHost.Name.
|
|
||
| Returns a boolean value indicating if the VMHost Power policy should be updated. | ||
| #> | ||
| [bool] ShouldUpdateVMHostPowerPolicy($vmHostCurrentPowerPolicy) |
There was a problem hiding this comment.
I think this method is redundant - you can just move the One line to the Test method.
|
|
||
| public override int GetHashCode() | ||
| { | ||
| if( this.CurrentPolicy == null ) { |
There was a problem hiding this comment.
Move the bracket on a new line.
| @@ -1,9 +1,9 @@ | |||
| <# | |||
There was a problem hiding this comment.
Remove this file from the PR as Travis will take care of updating the content of it.
| @@ -855,6 +855,66 @@ class VMHostNtpSettings : VMHostBaseDSC { | |||
| } | |||
| } | |||
|
|
|||
| [DscResource()] | |||
There was a problem hiding this comment.
The same comment as for the psd1 file.
|
@rockaut if you can update your pull request to merge into master instead of dev. (Or after you are ready create new PR). |
|
@rockaut as we announced last week today we are deleting the dev branch. So when you are ready with the latest changes from the review, please create new Pull Request to the master branch. To delete the dev branch we need to close all Pull Requests to it. Are you ok with closing this PR and creating new one to the master branch ? |
|
Ok will do.
Am Fr., 11. Jan. 2019, 13:43 hat SimeonGerginov <notifications@github.com>
geschrieben:
… @rockaut <https://github.com/rockaut> as we announced last week today we
are deleting the *dev* branch. So when you are ready with the latest
changes from the review, please create new Pull Request to the *master*
branch.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAbq-BgQFJyZUQUPIWNwlN4zJ3Wt1Nyuks5vCIcFgaJpZM4ZWZGs>
.
|
|
So @rockaut , can I close this one or will you close it? |
|
will redo on master |
Integrated new resource for VMHostPowerPolicy
Included Configuration file
Missing Unit tests because of issue #16
Signed-off-by: Markus Fischbacher fischbacher.markus@gmail.com