Overhauled 900_clone_users_and_groups.sh #1471
Conversation
|
Please wait - it is not yet tested... |
|
I tested it and it seems to work at least for me. |
|
If there are no furious objections ;-) |
There was a problem hiding this comment.
Very good of you to refactor this code - thanks!
I think that this will work but I am afraid that there is a fundamental change compared to the previous code.
If CLONE_ALL_USERS_GROUPS is set then this code actually replaces the CLONE_USERS and CLONE_GROUPS from the config. IMHO it should instead add the entries to the arrays set by the user.
The use case is actually adding users and groups that are not defined in the local files but can be resolved through NSS. The later code uses getent to retrieve the details so that this also works for remote users and groups.
I think that we should rename the CLONE_ALL_USERS_AND_GROUPS parameter to be called CLONE_ALL_LOCAL_USERS_AND_GROUPS to make it really explicit that this wildcard cloning is only about local users.
| test "${CLONE_USERS[*]}" && Log "Cloning users: ${CLONE_USERS[@]}" | ||
| for user in "${CLONE_USERS[@]}" ; do | ||
| # Skip if the user exists already in the ReaR recovery system: | ||
| grep "^$user:" $ROOTFS_DIR/etc/passwd 1>&2 && continue |
There was a problem hiding this comment.
No "grep -q" because that deletes information.
With "1>&2" I have the info in the log in any case
even if redirection of STDOUT gets again postponed.
There was a problem hiding this comment.
If grep fails then there is no output in STDOUT in any case. STDERR, where grep would complain about actual errors, is in any case already in the log. So I don't understand what you gain by this.
If grep found the line then I see no value in printing it because it is just the line that this script added there previously. If you want to dump the content of it then better do it with a suitable heading.
| fi | ||
| # Prepare to also add the user's group to the CLONE_GROUPS array: | ||
| # passwd_entry="user:password:UID:GID:description:HOMEdirectory:shell | ||
| groupID="$( echo "$passwd_entry" | cut -d ':' -f '4' )" |
There was a problem hiding this comment.
You could use cut ... <<<"$passwd_entry" to manage without the pipe
There was a problem hiding this comment.
Yes, thanks!
I am not yet used to use '<<<'.
| # group_entry="group:passwd:GID:userlist" | ||
| group="$( echo "$group_entry" | cut -d ':' -f '1' )" | ||
| # Skip if the group is already in the CLONE_GROUPS array: | ||
| IsInArray "$group" "${CLONE_GROUPS[@]}" && continue |
There was a problem hiding this comment.
I'd prefer here
if ! IsInArray "$group" "${CLONE_GROUPS[@]}" ; then
CLONE_GROUPS=( "${CLONE_GROUPS[@]}" "$group" )
fito make it easier to read. I would use continue only to denote skipping the entire while block for a certain item.
There was a problem hiding this comment.
I prefer "return early return often", cf.
https://github.com/rear/rear/wiki/Coding-Style
There was a problem hiding this comment.
For me "continue" means "continue with the next item"
and that is exactly what it should do in this case.
There was a problem hiding this comment.
Yes, it does that. However, the next developer who will add more stuff below will be easily fooled by that. That's why I find the use of continue in this case not optimum. With the if clause it is clear that the array check is related to the array addition. A developer can add code before or after that without any worries.
In other words: I prefer locally understandable logic over logic that jumps far away. I agree with the coding style piece you refer to but I don't agree that the generalization for while loops is a conclusion from that.
| test "${CLONE_GROUPS[*]}" && Log "Cloning groups: ${CLONE_GROUPS[@]}" | ||
| for group in "${CLONE_GROUPS[@]}" ; do | ||
| # Skip if the group exists already in the ReaR recovery system: | ||
| grep "^$group:" $ROOTFS_DIR/etc/group 1>&2 && continue |
|
@schlomo |
|
That CLONE_ALL_USERS_GROUPS |
jsmeix
deleted the
overhaul_900_clone_users_and_groups_related_to_pull_request_1464
branch
Now rescue/default/900_clone_users_and_groups.sh
should match https://github.com/rear/rear/wiki/Coding-Style
plus some possible bug fixes (more fail safe - hopefully)
and a better description in default.conf