-
Notifications
You must be signed in to change notification settings - Fork 140
fix(cli): check access process on windows #4293
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
Conversation
When running on windows, the process `username()` can contain the DOMAIN which makes the comparison fail.
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.
Pull Request Overview
This PR fixes a Windows-specific bug in the process access check where username comparison fails when the process username includes a domain prefix (e.g., "DOMAIN\username"). The fix extracts the username portion after the backslash on Windows systems.
Key changes:
- Added platform-specific handling for Windows usernames in
_can_access_process() - Extracts username from "DOMAIN\username" format by splitting on backslash
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
for more information, see https://pre-commit.ci
germa89
left a comment
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 forgot to mention, you should add a test for this please :)
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (66.66%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #4293 +/- ##
==========================================
+ Coverage 91.28% 91.34% +0.06%
==========================================
Files 193 193
Lines 15742 15745 +3
==========================================
+ Hits 14370 14383 +13
+ Misses 1372 1362 -10 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
|
@germa89 The current implementation of the tests do not allow me to test without having mapdl installed (or that's my understand from the need to provide a path for |
germa89
left a comment
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.
Thank you @SMoraisAnsys for this PR. It does fix one our current issues that went unnoticed.
Also sorry for the delay to get to this.
LGTM.
Close #4321
Description
When running on windows, the process
username()can contain the DOMAIN which makes the comparison fail.Issue linked
None
Checklist
draftif it is not ready to be reviewed yet.feat: adding new MAPDL command)