scottpas commented 6 years ago

This should resolve Issue #8

Copied the resources from https://github.com/Azure/RDS-Templates/tree/master/DSC/Configuration/xRemoteDesktopSessionHost/DSCResources and updated the OS version check to use the Test-xRemoteDesktopSessionHostOsRequirement function.

This change is

msftclas commented 6 years ago

All CLA requirements met.

codecov-io commented 6 years ago

Codecov Report

Merging #33 into dev will increase coverage by 3.51%. The diff coverage is 72.67%.

@@            Coverage Diff            @@
##              dev     #33      +/-   ##
=========================================
+ Coverage   65.59%   69.1%   +3.51%     
=========================================
  Files           5       8       +3     
  Lines         186     369     +183     
  Branches        4       9       +5     
=========================================
+ Hits          122     255     +133     
- Misses         60     105      +45     
- Partials        4       9       +5

Impacted Files	Coverage Δ
DSCResources/MSFT_xRDServer/MSFT_xRDServer.psm1	`51.61% <51.61%> (ø)`
...nseConfiguration/MSFT_xRDLicenseConfiguration.psm1	`75% <75%> (ø)`
...wayConfiguration/MSFT_xRDGatewayConfiguration.psm1	`88.31% <88.31%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d22bc83...38a473d. Read the comment docs.

johlju commented 6 years ago

@scottpas thanks for sending this in! I did a fast initial review and made a few review comments of the biggest items.

A question, the other resources in the quick-start template. Are they exactly like those already here, or do they have code changes that could be worth adding here as well? No need to do it in this PR, just checking you know, and if this closes issue #8 entirely or if it will be work left after this.

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, some commit checks failed.

a discussion (no related file): Are you up to making sure this code is up to the style guide line and best practices?

a discussion (no related file): Are you up to adding at least unit tests for these resources? See tests guidelines.

a discussion (no related file): The documentation (README.md) should be updated with these resources.

a discussion (no related file): Please update with an entry of this change in the change log, under the Unreleased section in the file README.md.

Comments from Reviewable

scottpas commented 6 years ago

I took a cursory look at the other resources and they appear to be from the 1.0.1 release of xRemoteDesktopSessionHost. I'm also working on addressing your other comments now

Comments from Reviewable

scottpas commented 6 years ago

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Are you up to making sure this code is up to the [style guide line](https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md) and [best practices](https://github.com/PowerShell/DscResources/blob/master/BestPractices.md)?

Everything should be updated appropriately now, per PSSA tests

Comments from Reviewable

scottpas commented 6 years ago

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

The documentation (README.md) should be updated with these resources.

Added descriptions for the new resources

Comments from Reviewable

scottpas commented 6 years ago

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Please update with an entry of this change in the change log, under the Unreleased section in the file README.md.

Done. Let me know if you think I should add a link to the quickstart files

Comments from Reviewable

ld0614 commented 6 years ago

Hi Guys, the code looks good, just to let you know that I'm just in final testing of a similar patch which includes a full copy from the quickstart repository as well as further fixes to enable HA deployments. My view on unit tests would be to get something merged into dev so that people can start taking advantage of the new functionality but make unit tests the next major priority for the whole repo

scottpas commented 6 years ago

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Are you up to adding at least unit tests for these resources? See [tests guidelines](https://github.com/PowerShell/DscResources/blob/master/TestsGuidelines.md).

I've added tests for xRDServer and xRDGatewayConfiguration

Comments from Reviewable

johlju commented 6 years ago

@ld0614 I understand your point, but I think adding the resources here (and code in general) should bring more value, and unit tests (and integration tests) are a big value for the user to be able to verify the code. Also when contributors make changes they, and the reviewer, can be comfortable that the change most likely did not break any existing functionality since tests passed. We should always deliver testable code. The backlog is pretty big, so my thought is that we should not be adding more work to it. I will enforce at least unit tests for the code that is changed before I merge changes. I think this bring great value, and sets a standard, and a comfortable level for the users to use these resources.

johlju commented 6 years ago

@ld0614 can you add to issue https://github.com/PowerShell/xRemoteDesktopSessionHost/issues/8 which resources you are working on. When this PR is merged you will have to base your work on the new dev branch. 🙂

johlju commented 6 years ago

Review status: 0 of 9 files reviewed at latest revision, 2 unresolved discussions.

a discussion (no related file):

Previously, scottpas (Scott Paschke) wrote…

Done. Let me know if you think I should add a link to the quickstart files

Please add a link, and give the authors credit (as per Michael's suggestion in the issue).

a discussion (no related file):

Previously, scottpas (Scott Paschke) wrote…

I've added tests for xRDServer and xRDGatewayConfiguration

Would you be up to adding tests for xRDLicenseConfiguration too?

Comments from Reviewable

johlju commented 6 years ago

@ld0614 would you mind continue drive the review of this PR? If so I will focus on reviews in other repos instead 🙂

ld0614 commented 6 years ago

@johlju Yes I'm happy to lead on this review, apologies that I've been a little silent around here for a few months.

ld0614 commented 6 years ago

Review status: 0 of 9 files reviewed at latest revision, 2 unresolved discussions.

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Please add a link, and give the authors credit (as per Michael's suggestion in the issue).

The change should be made under the unreleased section as versions are incremented during the release process, to be slightly picky Add should also probably be Added as it should be read as if the change was made in the past

Comments from Reviewable

johlju commented 6 years ago

@ld0614 No worries. I know we all have a lot to do! 😃

danielboth commented 6 years ago

Thanks @scottpas for this PR. I think the resources added here are very valuable! I still see some work to be done though, like adding tests for xRDLicenseConfiguration, do you plan to continue work on this? I'm happy to contribute by adding the tests if you like.

scottpas commented 6 years ago

@danielboth Sorry for the delay, I pushed up my changes to xRDLicenseConfiguration.

It appears that LicenseMode is a required parameter for the Set-RDLicenseConfiguration cmdlet, so I set it to mandatory for the Set/Test-TargetResource functions. On Get-TargetResource, it doesn't use LicenseMode at all, but it will fail the Script Resource Schema Validation pester tests if the parameters are not the same for all the TargetResource functions; however, the DSC best practice guide says that Get-TargetResource shouldn't contain any unused (or non-mandatory) parameters -- anyone have thoughts on this?

ld0614 commented 6 years ago

As the resource is a mandatory one I would suggest that you include it and just pass the response back, alternatively you could remove the requirement on mandatory and just set a default. By the looks of the code defaulting to notconfigured might be a good idea?

scottpas commented 6 years ago

Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions.

a discussion (no related file):

Previously, ld0614 (Leo D'Arcy) wrote…

The change should be made under the unreleased section as versions are incremented during the release process, to be slightly picky Add should also probably be Added as it should be read as if the change was made in the past

Done.

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Would you be up to adding tests for xRDLicenseConfiguration too?

Done.

Comments from Reviewable

ld0614 commented 6 years ago

@johlju are you happy for me to merge?

Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

ld0614 commented 6 years ago

@scottpas I've just merged another PR into xRemoteDesktopSessionHost which is now causing a conflict on your readme file, would you be able to resolve the conflict please?

ld0614 commented 6 years ago

@scottpas thanks for handling the merge conflict, as the previous PR also included significant testing your PR is now failing as your only testing 30% of your changes. Would you be able to add additional tests to your test files to test different scenarios please? Currently the bar is to include at least 66% coverage of changes. You can identify which lines aren't currently being tested here. An example of test code can now be located here

danielboth commented 6 years ago

Hi @scottpas, I noticed the issue of missing tests on the new resources and decided to add a few. I currently added those to https://github.com/danielboth/xRemoteDesktopSessionHost/tree/azure-quickstart-resources-tests. I also did one change in there where I renamed the LicenseServers parameter on the xRDLicenseConfiguration to LicenseServer (singular) as it should be in PowerShell.

I just created a PR to get those changes into your branch so we can go forward with this.

Hope you can continue on this PR?

scottpas commented 6 years ago

Thanks for writing the tests @danielboth! I merged them in and everything looks happy now.

danielboth commented 6 years ago

Indeed, look good now, thanks! @ld0614 can we merge this one in now?

ld0614 commented 6 years ago

We've just got to wait on @johlju to complete his comments and then I'll merge, thanks for the additional tests.

Review status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @johlju)

Comments from Reviewable

ld0614 commented 6 years ago

Reviewed 2 of 5 files at r4, 1 of 6 files at r5, 2 of 12 files at r9, 5 of 5 files at r11. Review status: all files reviewed, 2 unresolved discussions (waiting on @johlju)

Comments from Reviewable

johlju commented 6 years ago

@scottpas @danielboth awesome work both of you! @danielboth Thank you for helping writing the tests! :bowing_man:

Review status: :shipit: complete! all files reviewed, all discussions resolved

Comments from Reviewable

ld0614 commented 6 years ago

Sorry for the delayed merge, thanks @scottpas and @danielboth for your contribution 😄

dsccommunity / xRemoteDesktopSessionHost

Add Resources from Azure quickstart template #33

Codecov Report