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

add multiple domain support #3

Merged
merged 12 commits into from
Aug 14, 2023
Merged

add multiple domain support #3

merged 12 commits into from
Aug 14, 2023

Conversation

PoshAJ
Copy link
Collaborator

@PoshAJ PoshAJ commented Aug 11, 2023

Added support for situations with multiple domains such as children domains. This had the side effect of removing the requirement for the ActiveDirectory module.

Copy link
Owner

@santisq santisq left a comment

Choose a reason for hiding this comment

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

@PoshAJ I made a few changes to the source code. Removed anr filter too. Testing it on my side looks good, could you please test it on yours and let me know if it's working properly? If you have some tests groups and are willing to update the old screenshots feel free to do so. Thank you.

@santisq santisq self-assigned this Aug 13, 2023
@santisq santisq added documentation Improvements or additions to documentation enhancement New feature or request labels Aug 13, 2023
@PoshAJ
Copy link
Collaborator Author

PoshAJ commented Aug 13, 2023

@PoshAJ I made a few changes to the source code. Removed anr filter too. Testing it on my side looks good, could you please test it on yours and let me know if it's working properly? If you have some tests groups and are willing to update the old screenshots feel free to do so. Thank you.

Confirmed changes work with multiple domains, will take updated screenshots for README.md.

@santisq One thing of note is that now that it's cross domain, would we want to add that to the output? I.E DOMAIN\USER vice just USER.

@santisq
Copy link
Owner

santisq commented Aug 14, 2023

@PoshAJ I made a few changes to the source code. Removed anr filter too. Testing it on my side looks good, could you please test it on yours and let me know if it's working properly? If you have some tests groups and are willing to update the old screenshots feel free to do so. Thank you.

Confirmed changes work with multiple domains, will take updated screenshots for README.md.

@santisq One thing of note is that now that it's cross domain, would we want to add that to the output? I.E DOMAIN\USER vice just USER.

@PoshAJ Sounds good, you can add that change if you like then merge. I'll probably rewrite this function as a binary cmdlet later this week. The code is really old and needs improvement but definitely if you come up with more improvements make a new PR. I'm thinking I'll add a -Properties parameter too. Thanks.

@santisq santisq merged commit 5a97cc6 into santisq:main Aug 14, 2023
@santisq
Copy link
Owner

santisq commented Aug 14, 2023

looking good. thank you for your contribution @PoshAJ

@PoshAJ
Copy link
Collaborator Author

PoshAJ commented Aug 14, 2023

looking good. thank you for your contribution @PoshAJ

@santisq Not a problem :) if you need/want a collaborator for the rewrite, let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants