auto-configure PATH in shell profile after install#1064
auto-configure PATH in shell profile after install#1064VaibhavUpreti merged 2 commits intoTracer-Cloud:mainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Greptile SummaryThis PR adds a
Confidence Score: 4/5Safe to merge after fixing the marker-only idempotency guard that silently skips re-adding PATH when the export line has been deleted. One P1 logic bug: when the installer marker is present but the path export line was manually removed, configure_path returns silently without re-adding the path or informing the user. All other findings are P2 style/test gaps. The fix is a one-liner and the test suite otherwise provides solid coverage. install.sh lines 325-328 (marker-only idempotency check) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[configure_path called] --> B{INSTALL_DIR already in PATH env?}
B -- Yes --> Z[return]
B -- No --> C{platform = windows?}
C -- Yes --> D[warn: add to Git Bash / Windows PATH]
D --> Z
C -- No --> E[detect shell_name via $SHELL]
E --> F{shell_name?}
F -- zsh --> G[rc_file = ~/.zshrc]
F -- bash/linux --> H[rc_file = ~/.bashrc]
F -- bash/darwin --> I[rc_file = ~/.bash_profile]
F -- fish --> J[rc_file = ~/.config/fish/config.fish]
F -- unknown --> K[log manual export instruction]
K --> Z
G & H & I & J --> L[mkdir -p rc_dir if missing]
L --> M{INSTALL_DIR string in rc_file?}
M -- Yes --> Z
M -- No --> N{marker comment in rc_file?}
N -- Yes --> Z
N -- No --> O[append marker + path_line to rc_file]
O --> P[log source instruction to user]
P --> Z
Reviews (1): Last reviewed commit: "auto-configure PATH in shell profile aft..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| local marker="# Added by opensre installer" | ||
| if [ -f "$rc_file" ] && grep -qF "$marker" "$rc_file"; then | ||
| return | ||
| fi |
There was a problem hiding this comment.
If a user manually deletes the export PATH=... line from their rc file but leaves the # Added by opensre installer marker, the second guard fires and configure_path returns silently — no log output, no warning. The user re-runs the installer, it prints "Installed … to …" and exits cleanly, but opensre is still not on PATH. The idempotency check should only skip when the path line itself is present, not merely the marker.
| local marker="# Added by opensre installer" | |
| if [ -f "$rc_file" ] && grep -qF "$marker" "$rc_file"; then | |
| return | |
| fi | |
| local marker="# Added by opensre installer" | |
| if [ -f "$rc_file" ] && grep -qF "$marker" "$rc_file" && grep -qF "${INSTALL_DIR}" "$rc_file"; then | |
| return | |
| fi |
|
|
||
| log "" | ||
| log "${BIN_NAME:-opensre} has been added to PATH in ${rc_file}." | ||
| log "To apply now, run: source ${rc_file}" |
There was a problem hiding this comment.
source instruction
If rc_file contains spaces (e.g. a home directory like /home/john doe/.zshrc), the displayed command source /home/john doe/.zshrc is syntactically invalid. The path should be quoted.
| log "To apply now, run: source ${rc_file}" | |
| log "To apply now, run: source \"${rc_file}\"" |
| import textwrap | ||
| from pathlib import Path | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
pytest is imported but never referenced in the test file — no fixtures, marks, or pytest.raises calls. Can be removed.
| def test_idempotent_no_duplicate_on_rerun(tmp_path: Path) -> None: | ||
| _run(tmp_path, shell="/bin/zsh") | ||
| _run(tmp_path, shell="/bin/zsh") | ||
| content = (tmp_path / "home" / ".zshrc").read_text() | ||
| export_lines = [ln for ln in content.splitlines() if _LOCAL_BIN in ln and "export PATH" in ln] | ||
| assert len(export_lines) == 1 |
There was a problem hiding this comment.
The idempotency suite covers "second run adds no duplicate," but there is no test that verifies the installer re-adds the export line when the marker comment is already in the file but the export PATH line has been manually deleted. Adding this case would also serve as a regression guard for the logic fix above.
…_file in source hint
VaibhavUpreti
left a comment
VaibhavUpreti
left a comment
There was a problem hiding this comment.
cheers @rrajan94 ! Could you please make a task for someone to check for windows? I'm merging this
|
🐉 Legend says enough merged PRs and you ascend. @rrajan94 is dangerously close. 🌤️
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
…1099) (#1101) The post-merge ``test (windows-latest)`` job on commit 67dc64d (PR #1090 landing) is failing with two distinct symptoms in the install.sh test suites added by PR #1064: 1. ``tests/cli/test_install_sh_path.py`` — 12 tests fail because the subprocess call ``subprocess.run(["bash", "-c", script])`` resolves ``bash`` to ``wsl.exe`` on the GitHub Actions ``windows-latest`` runner, and that runner has no installed WSL distribution. Every ``_run`` call exits 1 with a literal "Windows Subsystem for Linux has no installed distributions" message, so none of the asserted rc files (``.zshrc``/``.bashrc``/``config.fish``) ever get written. 2. ``tests/cli/test_install_sh_resolution.py`` — module fails to import on Windows: the existing ``@pytest.mark.skipif(os.geteuid() == 0, reason=...)`` decorator evaluates ``os.geteuid()`` at decorator-application time, but ``os.geteuid`` does not exist on Windows. ``AttributeError`` fires *before* pytest can read any markers, blocking collection of every test in the module. Both files exclusively test ``install.sh``, which is a POSIX shell script (zsh/bash/fish RC files) with no Windows analogue, so the correct fix for the Windows runner is to skip them. Changes: - ``test_install_sh_path.py``: module-level ``pytestmark = pytest.mark.skipif(sys.platform == "win32", ...)`` with a comment that names the failure mode. - ``test_install_sh_resolution.py``: same module-level skipif, *and* defensively replace ``os.geteuid() == 0`` with ``_RUNNING_AS_ROOT = hasattr(os, "geteuid") and os.geteuid() == 0`` evaluated once at module load. ``hasattr`` short-circuits the ``and`` on Windows so ``os.geteuid()`` is never called there — the module imports cleanly even before pytest reads ``pytestmark``. No source-checkout behaviour change: skipif is a no-op on macOS/Linux, and the existing root-only test still skips correctly on those platforms (verified by running ``pytest tests/cli/test_install_sh_path.py tests/cli/test_install_sh_resolution.py`` locally — 15 / 15 pass). Full unit suite: 4164 pass / 2 skipped / 1 xfailed. Closes #1099.
…racer-Cloud#1099) (Tracer-Cloud#1101) The post-merge ``test (windows-latest)`` job on commit 67dc64d (PR Tracer-Cloud#1090 landing) is failing with two distinct symptoms in the install.sh test suites added by PR Tracer-Cloud#1064: 1. ``tests/cli/test_install_sh_path.py`` — 12 tests fail because the subprocess call ``subprocess.run(["bash", "-c", script])`` resolves ``bash`` to ``wsl.exe`` on the GitHub Actions ``windows-latest`` runner, and that runner has no installed WSL distribution. Every ``_run`` call exits 1 with a literal "Windows Subsystem for Linux has no installed distributions" message, so none of the asserted rc files (``.zshrc``/``.bashrc``/``config.fish``) ever get written. 2. ``tests/cli/test_install_sh_resolution.py`` — module fails to import on Windows: the existing ``@pytest.mark.skipif(os.geteuid() == 0, reason=...)`` decorator evaluates ``os.geteuid()`` at decorator-application time, but ``os.geteuid`` does not exist on Windows. ``AttributeError`` fires *before* pytest can read any markers, blocking collection of every test in the module. Both files exclusively test ``install.sh``, which is a POSIX shell script (zsh/bash/fish RC files) with no Windows analogue, so the correct fix for the Windows runner is to skip them. Changes: - ``test_install_sh_path.py``: module-level ``pytestmark = pytest.mark.skipif(sys.platform == "win32", ...)`` with a comment that names the failure mode. - ``test_install_sh_resolution.py``: same module-level skipif, *and* defensively replace ``os.geteuid() == 0`` with ``_RUNNING_AS_ROOT = hasattr(os, "geteuid") and os.geteuid() == 0`` evaluated once at module load. ``hasattr`` short-circuits the ``and`` on Windows so ``os.geteuid()`` is never called there — the module imports cleanly even before pytest reads ``pytestmark``. No source-checkout behaviour change: skipif is a no-op on macOS/Linux, and the existing root-only test still skips correctly on those platforms (verified by running ``pytest tests/cli/test_install_sh_path.py tests/cli/test_install_sh_resolution.py`` locally — 15 / 15 pass). Full unit suite: 4164 pass / 2 skipped / 1 xfailed. Closes Tracer-Cloud#1099.
3 participants
Fixes #1063
Describe the changes you have made in this PR -
After running the one-liner install,
opensregavecommand not foundbecause~/.local/binwasn't in$PATH. Users had to manually add the export line.This PR adds a
configure_path()function toinstall.shthat automatically writes the PATH export to the user's shell profile:~/.zshrc~/.bashrc~/.bash_profile~/.config/fish/config.fishusingfish_add_pathRe-running the installer never duplicates the line (idempotent via marker comment). The user is told to
source <profile>or open a new terminal after install.Demo/Screenshot for feature changes and bug fixes -
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
The install script was printing a warning asking users to manually add the PATH export. The fix detects the shell via
$SHELL, picks the right rc file per shell and OS, then appends an export line guarded by a marker comment so the operation is idempotent. Fish usesfish_add_pathinstead ofexport. Two checks prevent duplicate writes: one for the marker comment (installer-managed) and one for the install dir already appearing in the file (user-managed). The function is placed in the function-definition section of the script so it can be extracted and unit-tested in isolation — 11 tests cover all the above cases.Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.