Skip to content
/ wix Public

Comments

Util:user - Add domain runtime test skipping, and integration test including Domain="[valid domain]"#547

Closed
bevanweiss wants to merge 2 commits intowixtoolset:mainfrom
bevanweiss:util_user_getdomainserver_fixuseafterfree
Closed

Util:user - Add domain runtime test skipping, and integration test including Domain="[valid domain]"#547
bevanweiss wants to merge 2 commits intowixtoolset:mainfrom
bevanweiss:util_user_getdomainserver_fixuseafterfree

Conversation

@bevanweiss
Copy link
Contributor

@bevanweiss bevanweiss commented Jul 13, 2024

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:

  • nothing... just includes test revealing issue with Domain User creation

Copy link
Member

@robmen robmen left a 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.

  1. 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 #548.
  2. I left a comment in the 8629, but switching to Impersonate="yes" will break a lot. The existing CustomActions cannot be changed that way.

@bevanweiss
Copy link
Contributor Author

  1. 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.
  2. 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.
If there's anything left of the memory items, I'll put them into a different PR (the split).
And then after further testing (and logs to back it up if it works as I think I'm seeing) I'll squash the other commits back into this PR.

@bevanweiss bevanweiss marked this pull request as draft July 15, 2024 13:41
Copy link
Member

@robmen robmen left a 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.

@bevanweiss bevanweiss force-pushed the util_user_getdomainserver_fixuseafterfree branch from 1076df1 to 2851c44 Compare July 18, 2024 11:18
@bevanweiss bevanweiss changed the title Util:user - Fix a few memory allocation issues, fix domain user creation, and add domain runtime test skipping Util:user - Add domain runtime test skipping, and integration test including Domain="[valid domain]" Jul 18, 2024
@bevanweiss bevanweiss force-pushed the util_user_getdomainserver_fixuseafterfree branch 2 times, most recently from bb4549d to 088a2d3 Compare July 18, 2024 11:25
@bevanweiss bevanweiss marked this pull request as ready for review July 18, 2024 11:26
@bevanweiss bevanweiss requested a review from robmen July 22, 2024 04:31
Displayed appearance should be {domain}\{user} (as per Windows displays),
 not {domain}/{user}

Signed-off-by: Bevan Weiss <bevan.weiss@gmail.com>
This test currently fails under a Domain workstation.

Signed-off-by: Bevan Weiss <bevan.weiss@gmail.com>
@robmen robmen force-pushed the util_user_getdomainserver_fixuseafterfree branch from 088a2d3 to 54715f1 Compare December 26, 2024 15:18
@robmen
Copy link
Member

robmen commented Dec 26, 2024

@bevanweiss I'm uncertain what to do with the additional test given this statement in the commit message:

This test currently fails under a Domain workstation.

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.

@bevanweiss
Copy link
Contributor Author

@bevanweiss I'm uncertain what to do with the additional test given this statement in the commit message:

This test currently fails under a Domain workstation.

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.

@bevanweiss bevanweiss marked this pull request as draft December 26, 2024 19:04
@robmen robmen force-pushed the main branch 4 times, most recently from b5de212 to f181445 Compare February 12, 2025 23:09
@bevanweiss bevanweiss closed this Feb 16, 2025
@bevanweiss bevanweiss deleted the util_user_getdomainserver_fixuseafterfree branch February 16, 2025 05:04
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants