Skip to content
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

xADObjectPermissionEntry: Using more generic "AD:" to prevent errors #243

Merged
merged 22 commits into from
Mar 25, 2019

Conversation

DennisGaida
Copy link
Contributor

@DennisGaida DennisGaida commented Feb 28, 2019

As described in #236

Pull Request (PR) description

Using AD: is more generic than the currently used Microsoft.ActiveDirectory.Management\ActiveDirectory:://RootDSE/$Path which doesn't work on all machines (sometimes a .dll is added). Using AD: instead solves this problem.

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry under the Unreleased section of the change log in the README.md.
    Entry should say what was changed, and how that affects users (if applicable).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

kwirkykat and others added 16 commits March 30, 2016 15:01
Release of version 2.12.0.0 of xActiveDirectory
Release of version 2.17.0.0 of xActiveDirectory
Release of version 2.18.0.0 of xActiveDirectory
Release of version 2.19.0.0 of xActiveDirectory
Release of version 2.20.0.0 of xActiveDirectory
Release of version 2.21.0.0 of xActiveDirectory
Release of version 2.22.0.0 of xActiveDirectory
Release of version 2.23.0.0 of xActiveDirectory
Release of version 2.24.0.0 of xActiveDirectory
@codecov-io
Copy link

codecov-io commented Feb 28, 2019

Codecov Report

Merging #243 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #243   +/-   ##
===================================
  Coverage    85%    85%           
===================================
  Files        18     18           
  Lines      1553   1553           
  Branches     10     10           
===================================
  Hits       1324   1324           
  Misses      219    219           
  Partials     10     10

@johlju johlju added the needs review The pull request needs a code review. label Mar 2, 2019
@brwilkinson
Copy link

@DennisGaida looks good, are you okay to rebase and update?

@DennisGaida
Copy link
Contributor Author

I updated the PR with the latest changes from the dev branch

@johlju
Copy link
Member

johlju commented Mar 24, 2019

I just merged a PR that fixes the problem with the tests failing. Please rebase this PR again.

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! 😄 Just two review comments.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DennisGaida)

a discussion (no related file):
Could you please update the unreleased section under the change log in the file README.md.


a discussion (no related file):
Please make the same change in the unit test (replacing Microsoft.ActiveDirectory.Management\ActiveDirectory:://RootDSE):
https://github.com/PowerShell/xActiveDirectory/blob/dev/Tests/Unit/MSFT_xADObjectPermissionEntry.Tests.ps1


@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Mar 24, 2019
@msftclas
Copy link

msftclas commented Mar 25, 2019

CLA assistant check
All CLA requirements met.

Copy link
Contributor Author

@DennisGaida DennisGaida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @johlju)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please update the unreleased section under the change log in the file README.md.

Done.


a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Please make the same change in the unit test (replacing Microsoft.ActiveDirectory.Management\ActiveDirectory:://RootDSE):
https://github.com/PowerShell/xActiveDirectory/blob/dev/Tests/Unit/MSFT_xADObjectPermissionEntry.Tests.ps1

Done.


Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju johlju merged commit b9d12e1 into dsccommunity:dev Mar 25, 2019
@kwirkykat kwirkykat removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Mar 25, 2019
@johlju
Copy link
Member

johlju commented Mar 25, 2019

@DennisGaida Thanks for this!

johlju pushed a commit to johlju/ActiveDirectoryDsc that referenced this pull request Apr 19, 2019
…sccommunity#243)

- Updated xADObjectPermissionEntry to use `AD:` which is more
  generic when using `Get-Acl` and `Set-Acl` than using 
  `Microsoft.ActiveDirectory.Management\ActiveDirectory:://RootDSE/`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants