Repository navigation
Fix user support for LOCAL domain - #86
Merged
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdate admin user creation to use the correct /Local/Users home directory and shell-detection path when that directory exists, instead of /Users. Flow diagram for admin home directory and shell selectionflowchart TD
A[start save_selection] --> B[Get uname, name, password, shell]
B --> C{Directory /Local/Users exists?}
C -- Yes --> D[Set home_folder to /Local/Users/uname]
C -- No --> E[Set home_folder to /home/uname]
D --> F[Continue user creation]
E --> F[Continue user creation]
%% Shell selection logic in __init__
G[start __init__] --> H{Directory /Local/Users exists?}
H -- Yes --> I[Set sh to /usr/local/bin/zsh]
H -- No --> J[Set sh to /usr/local/bin/fish]
I --> K[Finish initialization]
J --> K[Finish initialization]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The logic for deciding the home directory and default shell is duplicated; consider extracting this into a small helper function so that the path and shell decision is defined in exactly one place.
- Hardcoding
/Local/Usersand/homemakes the code less portable; consider deriving the home directory from system information (e.g.,pwd.getpwnamorpathlib.Path.home()for the current user) or checking both possible base paths instead of assuming a single convention. - The conditional that sets the default shell based on the existence of
/Local/Usersmay not reflect the actual shell configured for the user; if possible, consider reading the user’s default shell from the system (e.g., viapwdor configuration) rather than inferring it from the directory layout.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for deciding the home directory and default shell is duplicated; consider extracting this into a small helper function so that the path and shell decision is defined in exactly one place.
- Hardcoding `/Local/Users` and `/home` makes the code less portable; consider deriving the home directory from system information (e.g., `pwd.getpwnam` or `pathlib.Path.home()` for the current user) or checking both possible base paths instead of assuming a single convention.
- The conditional that sets the default shell based on the existence of `/Local/Users` may not reflect the actual shell configured for the user; if possible, consider reading the user’s default shell from the system (e.g., via `pwd` or configuration) rather than inferring it from the directory layout.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Bug Fixes: