Skip to content

[FEAT] Added Automatic Flexibility for Regular and Root User - #284

Open
rhuanpk wants to merge 1 commit into
Raymo111:masterfrom
rhuanpk:master
Open

[FEAT] Added Automatic Flexibility for Regular and Root User#284
rhuanpk wants to merge 1 commit into
Raymo111:masterfrom
rhuanpk:master

Conversation

@rhuanpk

@rhuanpk rhuanpk commented Jul 31, 2023

Copy link
Copy Markdown

No description provided.

@rhuanpk rhuanpk changed the title Feat: added automatic flexibility for regular and root user. Feat: Added Automatic Flexibility for Regular and Root User Mar 13, 2024
@rhuanpk rhuanpk changed the title Feat: Added Automatic Flexibility for Regular and Root User [FEAT] Added Automatic Flexibility for Regular and Root User Mar 13, 2024

@Raymo111 Raymo111 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — Approve with nits

What it does: Introduces a SUDO variable set to sudo for normal users but unset when running as root ([ "$UID" -eq 0 ]), so make install runs directly as root instead of calling sudo (which may be absent in a root-only container).

Correctness: Root detection is sound — the operand is quoted and $UID is reliably populated under the #!/bin/bash shebang. Leaving $SUDO make install unquoted is the right idiom here: unset → it disappears via word-splitting yielding make install; set → sudo make install. (ShellCheck's SC2086 is a false positive in this pattern; quoting it would break the root case.) Non-root behavior is unchanged; the only change is dropping sudo when already root, which is a strict improvement.

Nits:

  • Empty PR description — a one-line rationale ("skip sudo when already root so it works in root-only containers") would help.
  • Optional: [ "$(id -u)" -eq 0 ] would make the check POSIX-portable if the script is ever run under a non-bash sh; not needed while the shebang stays bash.

Recommendation: Safe to merge — small, correct, behavior-improving; just add a one-line description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants