Skip to content

Adds AboutFiltering topic to the ActiveDirectory module - #343

Merged
vexx32 merged 16 commits into
PowerShellOrg:masterfrom
indented-automation:AddsActiveDirectoryModule
Jan 14, 2020
Merged

vexx32 merged 16 commits into
PowerShellOrg:masterfrom
indented-automation:AddsActiveDirectoryModule

Conversation

@indented-automation

Copy link
Copy Markdown
Contributor

PR Summary

Adds a module for ActiveDirectory with a short AboutFiltering topic.

Context

Attempts to cover frequently asked questions about filtering with the AD module.

Changes

Adds AboutFiltering.koans.ps1

Checklist

  • Pull Request has a meaningful title.
  • Summarised changes.
  • Pull Request is ready to merge & is not WIP.
  • Added tests / only testable interactively.
    • Make sure you add a new test if old tests do not effectively test the code changed.
  • Added documentation / opened issue to track adding documentation at a later date.

@vexx32 vexx32 added Category-Koans Invoking the Great Doubt PR-Needs-Review 🔍 Let's take a closer look! Module-ActiveDirectory Code or issues related to the Active Directory PowerShell module labels Jan 8, 2020

@steviecoaster steviecoaster left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, minus one small nitpick.

Comment thread PSKoans/Koans/Modules/ActiveDirectory/AboutFiltering.Koans.ps1 Outdated
steviecoaster
steviecoaster previously approved these changes Jan 12, 2020

@steviecoaster steviecoaster left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Awesome!

Comment thread PSKoans/Koans/Modules/ActiveDirectory/Introduction/AboutFiltering.Koans.ps1 Outdated
Comment thread PSKoans/Koans/Modules/ActiveDirectory/Introduction/AboutFiltering.Koans.ps1 Outdated

@vexx32 vexx32 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Found a few more bits and pieces that we should probably take care of before we merge. 🙂

Comment thread PSKoans/Koans/Modules/ActiveDirectory/Introduction/AboutFiltering.Koans.ps1 Outdated
Comment thread PSKoans/Koans/Modules/ActiveDirectory/Introduction/AboutFiltering.Koans.ps1 Outdated
Comment thread PSKoans/Koans/Modules/ActiveDirectory/Introduction/AboutFiltering.Koans.ps1 Outdated
Comment thread PSKoans/Koans/Modules/ActiveDirectory/Introduction/AboutFiltering.Koans.ps1 Outdated
Comment thread PSKoans/Koans/Modules/ActiveDirectory/Introduction/AboutFiltering.Koans.ps1 Outdated
vexx32
vexx32 previously approved these changes Jan 13, 2020

@vexx32 vexx32 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me!

I have one follow-up question that's more of a general concern with this module. Since it relies on external data, these koans are going to be more fragile to work with by nature. I'm not sure it makes sense at this point, but perhaps in future it may make more sense to Mock the AD cmdlets rather than operate with live data?

@indented-automation

Copy link
Copy Markdown
Contributor Author

Yeah, I agree. Have to see which direction it goes in, we don't get all that many questions about things other than its complex filter parsing.

Comment thread PSKoans/Koans/Modules/ActiveDirectory/Introduction/AboutFiltering.Koans.ps1 Outdated
@vexx32 vexx32 removed the PR-Needs-Review 🔍 Let's take a closer look! label Jan 13, 2020
@vexx32
vexx32 merged commit 59fb800 into PowerShellOrg:master Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Category-Koans Invoking the Great Doubt Module-ActiveDirectory Code or issues related to the Active Directory PowerShell module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants