-
Notifications
You must be signed in to change notification settings - Fork 291
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
Util:user - Add domain runtime test skipping, and integration test including Domain="[valid domain]" #547
base: main
Are you sure you want to change the base?
Conversation
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.
@bevanweiss You've been doing some good work in PRs and definitely helping uncover some bugs. Now, I wanted to include the fixes to memory issues described in 8576 into v5.0.1, so I refactored some of your work into PR #548. That refactoring makes it easy to port. I didn't want you to think I was ignoring the work you've done here.
Also, some general feedback to help get some of your PRs across the finish line.
- I would have preferred commits 1,2 and 6 combined and sent in a one-commit PR. That way the PR tackles a single issue with a minimal isolated change that can be reviewed and committed. That is essentially what I did in Fix faulty memory access in Util's User custom actions #548.
- I left a comment in the 8629, but switching to Impersonate="yes" will break a lot. The existing CustomActions cannot be changed that way.
Once your changes for the memory items are in, I'll look to squash (and maybe split) the commits I've got here. |
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.
I know you're planning to rebase this change. I had an additional idea about not adding too many [Fact]
types.
1076df1
to
2851c44
Compare
bb4549d
to
088a2d3
Compare
Displayed appearance should be {domain}\{user} (as per Windows displays), not {domain}/{user} Signed-off-by: Bevan Weiss <[email protected]>
This test currently fails under a Domain workstation. Signed-off-by: Bevan Weiss <[email protected]>
088a2d3
to
54715f1
Compare
@bevanweiss I'm uncertain what to do with the additional test given this statement in the commit message:
If the test is failing, we should not have it dormant, waiting to surprise someone (future us, mostly likely) when the tests are finally run on a domain. |
Once the Group Domain stuff is in, then I'll update the User stuff to align with it (i.e. separate Custom Actions for Domain vs Local actions). In light of that, I'll mark this PR as 'Draft', so it's a placeholder to remind me to do this. |
As the resolution of Domain User creation is a bigger fish than i'd like this week, i've dramatically scaled back this PR.
It now just introduces the Runtime conditional for domain membership for tests.
And adds a test (that if executed fails) showing the current deficiency in the User creation (when Domain is specified).
I'll use the Create Group changes to bring in altered treatment of Domain / Local Membership (in addition to the Domain / Local Group Creation / Removal).
Then it should be relatively easy to swing across the User handling in a subsequent PR.
Fixes: