dsccommunity / ComputerManagementDsc

DSC resources for for configuration of a Windows computer. These DSC resources allow you to perform computer management tasks, such as renaming the computer, joining a domain and scheduling tasks as well as configuring items such as virtual memory, event logs, time zones and power settings.
https://dsccommunity.org
MIT License
303 stars 83 forks source link

Adding Options parameter to Computer resource #381

Closed nickgw closed 2 years ago

nickgw commented 2 years ago

Pull Request (PR) description

Adds Options parameter to Computer resource, as defined in https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.management/add-computer?view=powershell-5.1#parameters

This Pull Request (PR) fixes the following issues

- Fixes #234 

Task list


This change is Reviewable

PlagueHO commented 2 years ago

Hi @nickgw - it looks like the pipelines are broken. I'll fix these this weekend so that the CI can run on your build. But in the meantime I'll do a quick review of your PR to get you some suggestions. Thank you for submitting!

PlagueHO commented 2 years ago

Once this PR (https://github.com/dsccommunity/ComputerManagementDsc/pull/384) is merged you should be able to rebase and the pipeline will work.

codecov[bot] commented 2 years ago

Codecov Report

Merging #381 (18c0c22) into main (f679412) will increase coverage by 0%. The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #381   +/-   ##
===================================
  Coverage    90%    90%           
===================================
  Files        17     17           
  Lines      1682   1690    +8     
===================================
+ Hits       1520   1528    +8     
  Misses      162    162           
Impacted Files Coverage Δ
source/DSCResources/DSC_Computer/DSC_Computer.psm1 89% <100%> (+<1%) :arrow_up:
PlagueHO commented 2 years ago

Hey @nickgw - your unit tests are failing because this Get-InvalidArgumentException should be Get-InvalidArgumentRecord.

PlagueHO commented 2 years ago

I'll have a look at test failures tomorrow night.

PlagueHO commented 2 years ago

Sorry about the delay in looking at this @nickgw - got a bit snowed under. Will try again tomorrow night.

nickgw commented 2 years ago

Sorry about the delay in looking at this @nickgw - got a bit snowed under. Will try again tomorrow night.

Thanks, appreciate your time!

nickgw commented 2 years ago

Hey @PlagueHO, please let me know when you get a chance to review this again!

PlagueHO commented 2 years ago

Hi @nickgw - sorry about the delay. I'll get onto this this weekend (snowed under with day job). I do notice that the code coverage has dropped ag bit - I think it just needs a unit test that covers these two lines by calling Set with the Options parameter defined.

PlagueHO commented 2 years ago

Cool - thanks @nickgw - I'll finish review this weekend!

PlagueHO commented 2 years ago

Looks like an intermittent build failure (integration tests on WS2022). So, I've kicked the build again and hopefully will deploy the preview release.