-
Notifications
You must be signed in to change notification settings - Fork 5
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
New resource FileSystemAccessRule #1
Conversation
@PlagueHO I trade you reviews. 😉 This is a big one so no hurry. |
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.
Did a quick review and spotted some quick changes.
Maybe worth for Daniel -eagle eye- Scott-Raynsford to double check though, as I went fast ;)
Reviewed 29 of 29 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @johlju and @PlagueHO)
CHANGELOG.md, line 13 at r1 (raw file):
- Added resource FileSystemAccessRule ### Changed
You probably want to clean up those not in use here?
RequiredModules.psd1, line 22 at r1 (raw file):
xDscResourceDesigner = 'latest' 'DscResource.DocGenerator' = 'latest' 'DscResource.Common' = '0.2.0'
Are you sure you want to pin DscResource.Common?
I'd prefer leaving to latest, and the day a new version of DscResource.Common breaks a build, pin it to previous working version until it's fixed.
tests/Integration/DSC_FileSystemAccessRule.Integration.Tests.ps1, line 11 at r1 (raw file):
catch [System.IO.FileNotFoundException] { throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -Tasks build" first.'
should this be -ResolveDependency
as well, or you just rely on the autoResolve?
tests/Unit/DSC_FileSystemAccessRule.Tests.ps1, line 12 at r1 (raw file):
catch [System.IO.FileNotFoundException] { throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -Tasks build" first.'
same
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: 27 of 29 files reviewed, 4 unresolved discussions (waiting on @gaelcolas and @PlagueHO)
CHANGELOG.md, line 13 at r1 (raw file):
Previously, gaelcolas (Gael) wrote…
You probably want to clean up those not in use here?
Done.
RequiredModules.psd1, line 22 at r1 (raw file):
Previously, gaelcolas (Gael) wrote…
Are you sure you want to pin DscResource.Common?
I'd prefer leaving to latest, and the day a new version of DscResource.Common breaks a build, pin it to previous working version until it's fixed.
Done.
tests/Integration/DSC_FileSystemAccessRule.Integration.Tests.ps1, line 11 at r1 (raw file):
Previously, gaelcolas (Gael) wrote…
should this be
-ResolveDependency
as well, or you just rely on the autoResolve?
I think something else (help doc) should mention the prereq for running build
? 🤔
tests/Unit/DSC_FileSystemAccessRule.Tests.ps1, line 12 at r1 (raw file):
Previously, gaelcolas (Gael) wrote…
same
I think something else (help doc) should mention the prereq for running build
? 🤔
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 5 of 29 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @gaelcolas and @johlju)
azure-pipelines.yml, line 49 at r2 (raw file):
publishLocation: 'Container' - stage: Test
Can you also run unit/integration tests in WS2016 as well?
azure-pipelines.yml, line 117 at r2 (raw file):
displayName: 'Integration' pool: vmImage: 'win1803'
Can you use windows-2019 instead?
tests/Integration/DSC_FileSystemAccessRule.config.ps1, line 2 at r2 (raw file):
$configFile = [System.IO.Path]::ChangeExtension($MyInvocation.MyCommand.Path, 'json') if (Test-Path -Path $configFile)
Add blank line before?
tests/Integration/DSC_FileSystemAccessRule.config.ps1, line 181 at r2 (raw file):
.SYNOPSIS Removes only the specified access rules for the identity when the identity in the current state have more rights than desired state.
have -> has
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 29 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gaelcolas and @johlju)
build.ps1, line 11 at r2 (raw file):
( [Parameter(Position = 0)] [string[]]$Tasks = '.',
Nit pick:
$Tasks on the next line...
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 22 of 29 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @gaelcolas and @johlju)
source/DSCResources/DSC_FileSystemAccessRule/DSC_FileSystemAccessRule.psm1, line 509 at r2 (raw file):
# If any rights that we want to deny are individually a full subset of existing rights. $currentRightResult = -not ($rightNotAllowed -eq ($rightNotAllowed -band ([System.Security.AccessControl.FileSystemRights] $currentState.Rights)))
Is there any way to reduce the length of this line?
source/DSCResources/DSC_FileSystemAccessRule/DSC_FileSystemAccessRule.schema.mof, line 4 at r2 (raw file):
class DSC_FileSystemAccessRule : OMI_BaseResource { [Key, Description("The path to the item that should have permissions set")] string Path;
Nitpick- end description with full stop.
source/DSCResources/DSC_FileSystemAccessRule/DSC_FileSystemAccessRule.schema.mof, line 5 at r2 (raw file):
{ [Key, Description("The path to the item that should have permissions set")] string Path; [Key, Description("The identity to set permissions for")] string Identity;
Nitpick- end description with full stop.
source/DSCResources/DSC_FileSystemAccessRule/en-US/DSC_FileSystemAccessRule.strings.psd1, line 12 at r2 (raw file):
NotPossibleClusterResourceOwner = The node '{0}' is not a possible owner for the path '{1}'. (FSAR0007) NoClusterDiskPartitionFound = No cluster disk partition was found that the path '{0}' could belong to. (FSAR0008) NodeIsNotClusterMember = 'Node does not belong to a Windows Server Failover Cluster. (FSAR0009)
There is a ' at the beginning that shouldn't be there.
source/Examples/README.md, line 4 at r2 (raw file):
This will help to understand how to setup certain scenarios with the FIleSystemDsx resource module.
FIleSystemDsx->FileSystemDsc
Also, capitalization problems.
tests/Unit/DSC_FileSystemAccessRule.Tests.ps1, line 251 at r2 (raw file):
Context 'When the configuration is present' { Context 'When the path exist' {
exists
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.
Thank you for these comments! Great catches! 😃
Reviewable status: 21 of 29 files reviewed, 14 unresolved discussions (waiting on @gaelcolas and @PlagueHO)
azure-pipelines.yml, line 49 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you also run unit/integration tests in WS2016 as well?
Done. I tried, hopefully the tests passes. 😄
azure-pipelines.yml, line 117 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Can you use windows-2019 instead?
Done.
build.ps1, line 11 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Nit pick:
$Tasks on the next line...
Done. Auto-formatted this as well. We need to fix this in sampler. See if I have time to go through that repo an clean it up.
source/DSCResources/DSC_FileSystemAccessRule/DSC_FileSystemAccessRule.psm1, line 509 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Is there any way to reduce the length of this line?
Done.
source/DSCResources/DSC_FileSystemAccessRule/DSC_FileSystemAccessRule.schema.mof, line 4 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Nitpick- end description with full stop.
Done.
source/DSCResources/DSC_FileSystemAccessRule/DSC_FileSystemAccessRule.schema.mof, line 5 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Nitpick- end description with full stop.
Done.
source/DSCResources/DSC_FileSystemAccessRule/en-US/DSC_FileSystemAccessRule.strings.psd1, line 12 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
There is a ' at the beginning that shouldn't be there.
Done.
source/Examples/README.md, line 4 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
FIleSystemDsx->FileSystemDsc
Also, capitalization problems.
Done.
tests/Integration/DSC_FileSystemAccessRule.config.ps1, line 2 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Add blank line before?
Done.
tests/Integration/DSC_FileSystemAccessRule.config.ps1, line 181 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
have -> has
Done.
tests/Unit/DSC_FileSystemAccessRule.Tests.ps1, line 251 at r2 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
exists
Done.
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 9 of 9 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gaelcolas)
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: all files reviewed, 4 unresolved discussions (waiting on @gaelcolas)
@PlagueHO cool! Thank you! 😄 |
This PR moves the resource xFileSystemAccessRule from the module xSystemSecurity to this module. This PR will make it possible to deprecate the module xSystemSecurity.
This change is