Revert "fix(atuin): record failed commands in history" - #1608
Conversation
This reverts commit 71c4373.
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes eliminate the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Mesa DescriptionTL;DRReverts the change that recorded failed commands in Atuin history. What changed?The changes introduced in #1606, which enabled recording of failed commands in Atuin history, have been reverted. Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the nix-cache-warmup script and its associated build steps, simplifies the Dockerfile installation process, and updates the Atuin configuration. Key feedback highlights a security risk where GITHUB_TOKEN is written directly to the Nix configuration in the Dockerfile, potentially exposing it in image layers. Additionally, the removal of error suppression in the Makefile and set -e in the Dockerfile reduces the robustness of the build process, and a discrepancy was noted in the Makefile's help description.
| if [ -n "$GITHUB_TOKEN" ]; then \ | ||
| echo "access-tokens = github.com=$GITHUB_TOKEN" >> /etc/nix/nix.conf ; \ | ||
| fi |
There was a problem hiding this comment.
Writing the GITHUB_TOKEN directly into /etc/nix/nix.conf within a RUN command persists the sensitive token in the image's filesystem and layer history. This is a security risk as the token can be extracted by anyone with access to the image. It is highly recommended to use Docker secrets (--mount=type=secret) to provide the token only during the build process without storing it in the final image.
| GIT_REMOTE_ORIGIN_URL := $(shell git config --get remote.origin.url) | ||
| GITHUB_REPO_PATH := $(shell echo $(GIT_REMOTE_ORIGIN_URL) | sed -n 's/.*github.com[:/]\(.*\)\.git/\1/p') | ||
| GITHUB_REPO_OWNER := $(shell echo $(GITHUB_REPO_PATH) | cut -d'/' -f1) | ||
| GITHUB_REPO_NAME := $(shell echo $(GITHUB_REPO_PATH) | cut -d'/' -f2) | ||
| GIT_COMMIT_SHA := $(shell git rev-parse --short HEAD 2>/dev/null || echo unknown) | ||
| GIT_COMMIT_SHA := $(shell git rev-parse --short HEAD) |
There was a problem hiding this comment.
Removing the error suppression (2>/dev/null) and the fallback value for GIT_COMMIT_SHA makes the Makefile less robust. In environments where git is not initialized or available, these commands will emit error messages to stderr on every make invocation, which can clutter output and potentially break scripts parsing the output.
GIT_REMOTE_ORIGIN_URL := $(shell git config --get remote.origin.url 2>/dev/null)
GITHUB_REPO_PATH := $(shell echo $(GIT_REMOTE_ORIGIN_URL) | sed -n 's/.*github.com[:/]\(.*\)\.git/\1/p')
GITHUB_REPO_OWNER := $(shell echo $(GITHUB_REPO_PATH) | cut -d'/' -f1)
GITHUB_REPO_NAME := $(shell echo $(GITHUB_REPO_PATH) | cut -d'/' -f2)
GIT_COMMIT_SHA := $(shell git rev-parse --short HEAD 2>/dev/null || echo unknown)
| RUN /usr/bin/nix-daemon & \ | ||
| sleep 5 && \ | ||
| # Run your dotfiles installation script. | ||
| # This script is expected to install fish and other tools. | ||
| # Make sure this script is idempotent or handles being run in a fresh environment. | ||
| sudo -u "$USER" -H env GITHUB_PR="$GITHUB_PR" GITHUB_TOKEN_FILE=/run/secrets/github_token IN_DOCKER=true USER="$USER" bash -c "curl -fsSL https://raw.githubusercontent.com/shunkakinoki/dotfiles/$COMMIT_SHA/install.sh | bash" | ||
| sudo -u $USER -E -H bash -c "curl -fsSL https://raw.githubusercontent.com/shunkakinoki/dotfiles/$COMMIT_SHA/install.sh | bash" |
There was a problem hiding this comment.
The set -e instruction was removed from this RUN block. It is best practice to include it to ensure the build fails immediately if any command in the sequence (such as the curl or bash execution) fails. Additionally, quoting $USER is recommended to prevent shell expansion issues if the username contains special characters or spaces.
RUN set -e; \
/usr/bin/nix-daemon & \
sleep 5 && \
# Run your dotfiles installation script.
# This script is expected to install fish and other tools.
# Make sure this script is idempotent or handles being run in a fresh environment.
sudo -u "$USER" -E -H bash -c "curl -fsSL https://raw.githubusercontent.com/shunkakinoki/dotfiles/$COMMIT_SHA/install.sh | bash"
|
|
||
| .PHONY: install | ||
| install: setup git-submodule-sync nix-cache-warmup nix-build nix-switch shell-install ## Set up full environment (setup, cache warmup, build, switch, shell-install). | ||
| install: setup git-submodule-sync nix-build nix-switch shell-install ## Set up full environment (setup, flake-update, build, switch, shell-install). |
There was a problem hiding this comment.
The help description for the install target mentions flake-update, but the nix-flake-update target is not included in the prerequisites list. The description should be updated to accurately reflect the actions performed by the target.
install: setup git-submodule-sync nix-build nix-switch shell-install ## Set up full environment (setup, submodule-sync, build, switch, shell-install).
1 participant
Reverts #1606
Summary by cubic
Reverts the
atuinhistory change and removes the Nix cache warmup/token plumbing to simplify builds and CI. Restores default history behavior and streamlines Docker/CI configuration.Bug Fixes
atuin: removehistory.exit_code = "all"so failed commands are not recorded.Refactors
scripts/nix-cache-warmup.sh, its Makefile target, and related tests/coverage.GITHUB_TOKENbuild-arg (written tonix.confif present), use registry-based build cache, always push images, and remove.dockerignore.NIX_CONFIGininstall.sh.Written for commit 85ea1d4. Summary will update on new commits. Review in cubic