-
Notifications
You must be signed in to change notification settings - Fork 141
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
xActiveDirectory: Opt-in to "Common Tests - Validate example files" #309
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #309 +/- ##
===================================
Coverage 92% 92%
===================================
Files 20 20
Lines 2307 2307
Branches 10 10
===================================
Hits 2143 2143
Misses 154 154
Partials 10 10 |
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.
Reviewed 27 of 33 files at r2, 6 of 6 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @X-Guardian)
.MetaTestOptIn.json, line 12 at r3 (raw file):
"Common Tests - Validate Example Files To Be Published"
Since we opt-in to this. Would you mind preparing the example to be published? It can be another PR if you like. It could also be a anopther PR because maybe we should publish the examples once the module is renamed?
- adding the PSScriptInfo block
- adding the
DESCRIPTION
in the comment-based help - adding the
#Requires
statement. - Rename the examples filename and configuration name to be prefixed with the resource name, and suffixed with
_Config
.
See example here: https://github.com/PowerShell/xActiveDirectory/blob/dev/Examples/Resources/xADComputer/1-AddComputerAccount_Config.ps1
We could then opt-in for publishing by adding the deploy step:
https://github.com/PowerShell/DscResource.Tests#deploy
Examples/Resources/xADDomainController/1-AddDomainControllerToDomainMinimal.ps1, line 25 at r3 (raw file):
xADDomainController_AddDomainControllerToDomainMinimal_Config
This is meant to be named like this to support publishing of this example. See previous comment. Although the file name should have been changed to match.
1-xADDomainController_AddDomainControllerToDomainMinimal_Config.ps1
Examples/Resources/xADDomainController/2-AddDomainControllerToDomainAllProperties.ps1, line 25 at r3 (raw file):
xADDomainController_AddDomainControllerToDomainAllProperties_Config
See previous comment.
Examples/Resources/xADDomainController/3-AddDomainControllerToDomainUsingIFM.ps1, line 25 at r3 (raw file):
xADDomainController_AddDomainControllerToDomainUsingIFM_Config
See previous comment.
OK, how about we remove the "Common Tests - Validate Example Files To Be Published" from this PR then, get this merged, then I'll create another PR with all the changes needed for the publishing? |
@X-Guardian that works for me. |
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.
Reviewable status: 32 of 33 files reviewed, 4 unresolved discussions (waiting on @johlju)
.MetaTestOptIn.json, line 12 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
"Common Tests - Validate Example Files To Be Published"
Since we opt-in to this. Would you mind preparing the example to be published? It can be another PR if you like. It could also be a anopther PR because maybe we should publish the examples once the module is renamed?
- adding the PSScriptInfo block
- adding the
DESCRIPTION
in the comment-based help- adding the
#Requires
statement.- Rename the examples filename and configuration name to be prefixed with the resource name, and suffixed with
_Config
.See example here: https://github.com/PowerShell/xActiveDirectory/blob/dev/Examples/Resources/xADComputer/1-AddComputerAccount_Config.ps1
We could then opt-in for publishing by adding the deploy step:
https://github.com/PowerShell/DscResource.Tests#deploy
Agreed to implement this in a separate PR
Examples/Resources/xADDomainController/1-AddDomainControllerToDomainMinimal.ps1, line 25 at r3 (raw file):
Agreed to implement this in a separate PR
Examples/Resources/xADDomainController/2-AddDomainControllerToDomainAllProperties.ps1, line 25 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
xADDomainController_AddDomainControllerToDomainAllProperties_Config
See previous comment.
Agreed to implement this in a separate PR
Examples/Resources/xADDomainController/3-AddDomainControllerToDomainUsingIFM.ps1, line 25 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
xADDomainController_AddDomainControllerToDomainUsingIFM_Config
See previous comment.
Agreed to implement this in a separate PR
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.
Reviewed 1 of 2 files at r4.
Reviewable status: 33 of 34 files reviewed, all discussions resolved
Just need a rebase and I merge this one. |
@johlju, can you resolve these conflicts online for me? I just get a spinning logo when I try. Otherwise it will have to wait till I get home this evening. |
- Remove incorrect DnsDelegationCredential - Rename ConfigurationData variable
- Add "Common Tests - Validate Example Files To Be Published"
- Remove "Common Tests - Validate Example Files To Be Published"
@X-Guardian I rebased the PR. |
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.
Reviewed 1 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved
Pull Request (PR) description
This PR opts-in to the "Common Tests - Validate example files" DSC Resource tests.
Summary of changes:
.MetaTestOptIn.json
TrustDirection
parameter and configuration data fromxADDomainTrust
NewOneWayTrust
exampleThis Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is