Skip to content

Conversation

junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Oct 3, 2025

Description

This change improves how the username is detected when tagging the clp‑package development Docker image, addressing issue #1371:

  1. get user from env var USER
  2. if that fails (likely due to the variable being missing from the terminal environment), run whoami
  3. if that still fails (likely due to the whoami binary missing), run id --user --name
  4. if that still fails (likely due to the username missing from /etc/passwd), run id --user to just get the user id
  5. if that still fails (likely due to the idbinary missing), use unknown as the fallback.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  1. Ran the script in a typical environment (WSL Ubuntu 22.04 + Docker Desktop), and found the image was tagged with clp-package:dev-<username>-<hash> by checking docker image ls
  2. Ran the script in a docker contained environment where the USER env var is missing and the username is missing from /etc/passwd, and found the image was tagged with clp-package:dev-<username-id>-<hash> by checking docker image ls:
    docker run -it \
        -v /var/run/docker.sock:/var/run/docker.sock \
        -v /usr/bin/docker:/usr/bin/docker:ro \
        -v /usr/libexec/docker/cli-plugins:/usr/libexec/docker/cli-plugins:ro \
        -v /home/$(id -un):/home/$(id -un) \
        --user $(id -u):$(getent group docker | cut -d: -f3) \
        -e HOME=/home/$(id -u) \
        -e TERM=$TERM \
        -w /home/$(id -u) \
        ghcr.io/y-scope/clp/clp-core-dependencies-x86-ubuntu-jammy:main \
        /bin/bash --login

Summary by CodeRabbit

  • Chores
    • Improved Docker build script to reliably detect the current user (with a fallback to “unknown”) and apply it to dev image tags as dev-{user}-{short_id}, enhancing consistency and traceability of locally built images.
    • No runtime behaviour or public API changes.

@junhaoliao junhaoliao requested a review from a team as a code owner October 3, 2025 01:13
Copy link
Contributor

coderabbitai bot commented Oct 3, 2025

Walkthrough

Adjusts the build script’s user resolution order to whoami, then id --user --name, else "unknown", and uses the resolved value in the Docker image tag as dev-${user}-${short_id}. No other logic paths are changed.

Changes

Cohort / File(s) Summary
Build script user resolution and tagging
tools/docker-images/clp-package/build.sh
Reorders username detection: whoami → id --user --name → "unknown"; updates command substitution formatting; applies resolved user in tag format dev-${user}-${short_id}.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit's high-level summary is enabled.
Title Check ✅ Passed The title clearly and concisely summarises the primary change—improving Docker user name detection when tagging the clp-package development image—and references the related issue.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 632eecb and 68665bd.

📒 Files selected for processing (1)
  • tools/docker-images/clp-package/build.sh (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: package-image
  • GitHub Check: rust-checks (ubuntu-24.04)
  • GitHub Check: rust-checks (ubuntu-22.04)
  • GitHub Check: rust-checks (macos-15)
  • GitHub Check: lint-check (ubuntu-24.04)
  • GitHub Check: lint-check (macos-15)

Comment on lines +51 to +54
user="${USER:-$(whoami 2>/dev/null)}" \
|| user=$(id --user --name 2>/dev/null) \
|| user=$(id --user 2>/dev/null) \
|| user="unknown";
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Restore POSIX-friendly id fallbacks.

id --user --name/id --user are GNU-only; macOS and BusyBox reject them, so the script now falls all the way to "unknown" on those platforms. The previous id -un/id -u path worked there, so this is a regression. Please retain the short-option fallbacks (or probe for GNU support) before giving up on resolving the username.

-    user="${USER:-$(whoami 2>/dev/null)}" \
-        || user=$(id --user --name 2>/dev/null) \
-        || user=$(id --user 2>/dev/null) \
+    user="${USER:-$(whoami 2>/dev/null)}" \
+        || user=$(id --user --name 2>/dev/null) \
+        || user=$(id -un 2>/dev/null) \
+        || user=$(id --user 2>/dev/null) \
+        || user=$(id -u 2>/dev/null) \
         || user="unknown";
🤖 Prompt for AI Agents
In tools/docker-images/clp-package/build.sh around lines 51-54, the current
fallbacks use GNU-only long id options and break on macOS/BusyBox; restore
POSIX-friendly fallbacks by trying id -un and id -u first (then the GNU
long-option forms if desired), then whoami as a next fallback, and finally
"unknown"; implement these attempts with || chains so the script resolves the
username on non-GNU systems before giving up.

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.

1 participant