-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add validation of CollectionName length 1-15 characters and tests #15
Conversation
…d tests to validate that invalid length throws an exception
Hi @JimPriestley, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
@TravisEz13 Changes from #13 are now here. |
Export-ModuleMember -Function *-TargetResource | ||
|
||
Import-Module -Name "$PSScriptRoot\..\..\xRemoteDesktopSessionHostCommon.psm1" | ||
if (!(Test-xRemoteDesktopSessionHostOsRequirement)) { Throw "The minimum OS requirement was not met."} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import is not a good place to throw an error. It will basically be hidden. This should be turned into a function and called at the beginning of test set and get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TravisEz13 You wrote that code and committed it on Feb 23, 2016. It is not part of the code I wrote. It is also not a test, it appears to be part of the functional code to block the resource being deployed on an OS that does not support the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, we can just file an issue for it.
This change involves a formatting change. Can you submit the formatting change separately from the code changes. I cannot tell what changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filed #16
Reviewed 8 of 8 files at r1. Comments from Reviewable |
Resubmission of Issue 10
This change is