dsccommunity / xRemoteDesktopSessionHost

This module contains DSC resources for the management and configuration of Microsoft Remote Desktop Session Host (RDSH).
MIT License
36 stars 47 forks source link

Re-submitting Change to Fix issue with resource in WIndows 10/Server 2016 #5

Closed ghost closed 8 years ago

ghost commented 8 years ago

Current version check will error with version number greater than 10, and so will not run on server 2016 or windows 10. Changed version check logic to allow this.

Re-submitting with Readme updated this time.

msftclas commented 8 years ago

Hi @samcogan82, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

TravisEz13 commented 8 years ago

Please resolve all merge conflicts. For more information see GitHowTo - 30. Resolving Conflicts Instructions to setup BC4, my favorite merge tool

It is required that you provide adequate test coverage for the code you change. Contribution Guidelines -Adding test coverage for DSC resources Please work with us to come up with a plan to cover this code.

Suggestion: Base the tests on the Unit tests Template

ghost commented 8 years ago

Hello, I am not showing any merge conflicts, however I am not particularly expert in Git so if there is an issue if you could point me in the right direction that would be appreciated.

Thanks

Sam

On Sun, Feb 21, 2016 at 11:30 PM, Travis Plunk notifications@github.com wrote:

Please resolve all merge conflicts. For more information see GitHowTo -

  1. Resolving Conflicts http://githowto.com/resolving_conflicts Instructions to setup BC4, my favorite merge tool http://www.scootersoftware.com/support.php?zz=kb_vcs#gitwindows

It is required that you provide adequate test coverage for the code you change. Contribution Guidelines -Adding test coverage for DSC resources https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md#adding-test-coverage-for-dsc-resources Please work with us to come up with a plan to cover this code.

Suggestion: Base the tests on the Unit tests Template https://github.com/PowerShell/DscResources/blob/master/Tests.Template/unit_template.ps1

— Reply to this email directly or view it on GitHub https://github.com/PowerShell/xRemoteDesktopSessionHost/pull/5#issuecomment-186943756 .

ghost commented 8 years ago

Ignore the previous email, I have found the issue and resolve the conflict.

On Mon, Feb 22, 2016 at 1:29 PM, Sam Cogan mail@samcogan.com wrote:

Hello, I am not showing any merge conflicts, however I am not particularly expert in Git so if there is an issue if you could point me in the right direction that would be appreciated.

Thanks

Sam

On Sun, Feb 21, 2016 at 11:30 PM, Travis Plunk notifications@github.com wrote:

Please resolve all merge conflicts. For more information see GitHowTo -

  1. Resolving Conflicts http://githowto.com/resolving_conflicts Instructions to setup BC4, my favorite merge tool http://www.scootersoftware.com/support.php?zz=kb_vcs#gitwindows

It is required that you provide adequate test coverage for the code you change. Contribution Guidelines -Adding test coverage for DSC resources https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md#adding-test-coverage-for-dsc-resources Please work with us to come up with a plan to cover this code.

Suggestion: Base the tests on the Unit tests Template https://github.com/PowerShell/DscResources/blob/master/Tests.Template/unit_template.ps1

— Reply to this email directly or view it on GitHub https://github.com/PowerShell/xRemoteDesktopSessionHost/pull/5#issuecomment-186943756 .

TravisEz13 commented 8 years ago

One issue not addressed

It is required that you provide adequate test coverage for the code you change. Contribution Guidelines -Adding test coverage for DSC resources Please work with us to come up with a plan to cover this code.

Suggestion: Base the tests on the Unit tests Template

Additionally, since the code is repeated, it would be good to move it into a common module.

ghost commented 8 years ago

I'm a bit confused here, this is a small change to an existing module that has been produced by Microsoft. As far as I can see there are no existing tests for this module at all, and it would seem odd to create a test just for this small item. Similarly as this is one line of code it would see silly to try and turn this into a module.

On Wed, Feb 24, 2016 at 3:32 AM, Travis Plunk notifications@github.com wrote:

One issue not addressed

It is required that you provide adequate test coverage for the code you change. Contribution Guidelines -Adding test coverage for DSC resources https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md#adding-test-coverage-for-dsc-resources Please work with us to come up with a plan to cover this code.

Suggestion: Base the tests on the Unit tests Template https://github.com/PowerShell/DscResources/blob/master/Tests.Template/unit_template.ps1

Additionally, since the code is repeated, it would be good to move it into a common module.

— Reply to this email directly or view it on GitHub https://github.com/PowerShell/xRemoteDesktopSessionHost/pull/5#issuecomment-188045133 .

TravisEz13 commented 8 years ago

@samcogan82 We have Contribution Guidelines -Adding test coverage for DSC resources to ensure that we maintain and improve the quality of the resources.

I understand who ever wrote the module did not provide tests, but we need tests to keep quality of the resources up. I discovered additional issues trying to write tests for this change. It's not nessecary to fix those issues, but testing your change and filing issues is important. Adding the test suite encourages others to add tests.

Putting the code in a common location prevents having to fix the same issue in multiple places. Looking at the code, this change isn't the only code that needs to be moved to a common location so making this change will encorage others to refactor common code to the common module.

I cloned your repo and tested your change and therefore I have submitted a PR to your repo with suggested changes.

ghost commented 8 years ago

Thanks Travis, I understand now, and that is really useful for looking at changes going forward, thanks for doing that. I have merged the change so it should be ready to go.

Thanks

Sam

On Thu, Feb 25, 2016 at 12:07 AM, Travis Plunk notifications@github.com wrote:

@samcogan82 https://github.com/samcogan82 We have Contribution Guidelines -Adding test coverage for DSC resources https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md to ensure that we maintain and improve the quality of the resources.

I understand who ever wrote the module did not provide tests, but we need tests to keep quality of the resources up. I discovered additional issues trying to write tests for this change. It's not nessecary to fix those issues, but testing your change and filing issues is important. Adding the test suite encourages others to add tests.

Putting the code in a common location prevents having to fix the same issue in multiple places. Looking at the code, this change isn't the only code that needs to be moved to a common location so making this change will encorage others to refactor common code to the common module.

I cloned your repo and tested your change and therefore I have submitted a PR to your repo with suggested changes.

— Reply to this email directly or view it on GitHub https://github.com/PowerShell/xRemoteDesktopSessionHost/pull/5#issuecomment-188528204 .

TravisEz13 commented 8 years ago

Thanks for your contribution!