Fix run_ci_local.sh to not prompt for username/password#792
Fix run_ci_local.sh to not prompt for username/password#792rapids-bot[bot] merged 4 commits intoNVIDIA:developfrom
run_ci_local.sh to not prompt for username/password#792Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
…e SSH_AUTH_SOCK to the container Signed-off-by: David Gardner <dagardner@nvidia.com>
WalkthroughUpdates CI scripts to support SSH-based Git operations when an SSH agent is available, including pre-emptive SSH host key scanning, controlled clone/checkout steps, environment variable exports, conditional URL transformation to HTTPS fallback, container SSH agent mounting, and added logging for LFS operations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant run_ci_local.sh as run_ci_local.sh
participant Docker as CI Container
participant bootstrap as bootstrap_local_ci.sh
participant Git as Git Server
participant SSH as SSH Agent
Dev->>run_ci_local.sh: Invoke with target
alt SSH_AUTH_SOCK set
run_ci_local.sh->>run_ci_local.sh: Keep SSH URLs
run_ci_local.sh->>Docker: docker run -v $SSH_AUTH_SOCK:/ssh-agent (ro)
run_ci_local.sh->>Docker: Set SSH_AUTH_SOCK=/ssh-agent
else No SSH agent
run_ci_local.sh->>run_ci_local.sh: Convert origin/upstream to HTTPS
run_ci_local.sh->>Docker: docker run (no SSH agent)
end
Docker->>bootstrap: Start CI bootstrap
alt SSH_AUTH_SOCK set in container
bootstrap->>Git: ssh-keyscan known hosts (no prompt)
end
opt USE_HOST_GIT not set
bootstrap->>Git: git clone -q
bootstrap->>Git: git remote add upstream
bootstrap->>Git: git fetch upstream
bootstrap->>Git: checkout develop → ${GIT_BRANCH} → pull
bootstrap->>Git: checkout ${GIT_COMMIT}
bootstrap->>Git: fetch --tags
bootstrap->>bootstrap: export CURRENT_BRANCH, COMMIT_SHA
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
ci/scripts/common.sh (1)
173-187: Avoid installing git-lfs when USE_HOST_GIT=1.Currently apt installs run unconditionally, then we log “skipping git-lfs install,” which isn’t true. Gate installs behind the flag to save time and avoid side effects in host‑git mode.
Apply:
-function get_lfs_files() { - rapids-logger "Installing git-lfs from apt" - apt update - apt install --no-install-recommends -y git-lfs - - if [[ "${USE_HOST_GIT}" == "1" ]]; then - rapids-logger "Using host git, skipping git-lfs install" - else +function get_lfs_files() { + if [[ "${USE_HOST_GIT}" == "1" ]]; then + rapids-logger "Using host git, skipping git-lfs install" + else + rapids-logger "Installing git-lfs from apt" + apt update + apt install --no-install-recommends -y git-lfs rapids-logger "Fetching LFS files" git lfs install rapids-logger "Calling git lfs fetch" git lfs fetch rapids-logger "Calling git lfs pull" git lfs pull fici/scripts/bootstrap_local_ci.sh (3)
17-20: Harden SSH known_hosts handling; avoid writing to /etc.Writing to /etc/ssh/ssh_known_hosts assumes root and trusts TOFU. Prefer user-level known_hosts and pin key type; hash entries for privacy.
-if [[ -n "${SSH_AUTH_SOCK}" ]]; then - # Avoids SSH host key verification prompt - ssh-keyscan github.com >> /etc/ssh/ssh_known_hosts -fi +if [[ -n "${SSH_AUTH_SOCK}" ]]; then + # Avoid SSH host key prompt: add GitHub host key to user known_hosts + mkdir -p ~/.ssh && chmod 700 ~/.ssh + # Pin ED25519 (GitHub primary) and hash entries + if ! grep -q "^\\|1\\|" ~/.ssh/known_hosts 2>/dev/null; then + ssh-keyscan -t ed25519 github.com 2>/dev/null | ssh-keygen -l -f - >/dev/null + fi + ssh-keyscan -H -t ed25519 github.com 2>/dev/null >> ~/.ssh/known_hosts + chmod 600 ~/.ssh/known_hosts +fi
23-31: Make cd robust (ShellCheck SC2164).Guard cd with exit so failures don’t proceed with a bad CWD.
- cd nat/ + cd nat/ || exit 1 @@ - cd nat/ + cd nat/ || exit 1
27-37: Consider explicit remotes/branches for deterministic checkout.Minor: ‘git pull’ relies on default remote/branch; make it explicit to avoid surprises.
- git checkout ${GIT_BRANCH} - git pull + git checkout ${GIT_BRANCH} + git pull --ff-only origin ${GIT_BRANCH}ci/scripts/run_ci_local.sh (3)
35-39: Broaden SSH→HTTPS conversion to cover ssh:// form.Handle both “git@github.com:owner/repo(.git)” and “ssh://git@github.com/owner/repo(.git)”.
function git_ssh_to_https() { local url=$1 - echo $url | sed -e 's|^git@github\.com:|https://github.com/|' + echo "$url" | sed -E 's|^(ssh://)?git@github\.com[:/]|https://github.com/|' }
86-89: Quote and validate SSH_AUTH_SOCK; ensure it’s a socket.Avoid word-splitting and mount only if the path is a socket.
- if [[ -n "${SSH_AUTH_SOCK}" ]]; then - DOCKER_RUN_ARGS="${DOCKER_RUN_ARGS} -v $(readlink -f $SSH_AUTH_SOCK):/ssh-agent:ro --env SSH_AUTH_SOCK=/ssh-agent" + if [[ -n "${SSH_AUTH_SOCK}" && -S "${SSH_AUTH_SOCK}" ]]; then + DOCKER_RUN_ARGS="${DOCKER_RUN_ARGS} -v $(readlink -f "${SSH_AUTH_SOCK}"):/ssh-agent:ro --env SSH_AUTH_SOCK=/ssh-agent" fi
80-84: Prefer arrays for docker args to avoid quoting pitfalls.Building a single string with embedded quotes is fragile. Arrays simplify spaces/quoting in paths and env values.
- DOCKER_RUN_ARGS="--rm -ti --net=host --platform=linux/${CI_ARCH} -v "${LOCAL_CI_TMP}":/ci_tmp ${ENV_LIST} --env STAGE=${STAGE}" + DOCKER_RUN_ARGS=(--rm -ti --net=host --platform=linux/${CI_ARCH} -v "${LOCAL_CI_TMP}":/ci_tmp) + # shellcheck disable=SC2206 # intentional split of ENV_LIST into array + DOCKER_RUN_ARGS+=(${ENV_LIST}) + DOCKER_RUN_ARGS+=(--env STAGE=${STAGE}) @@ - docker run ${DOCKER_RUN_ARGS} ${DOCKER_EXTRA_ARGS} ${CI_CONTAINER} ${DOCKER_RUN_CMD} + docker run "${DOCKER_RUN_ARGS[@]}" ${DOCKER_EXTRA_ARGS} ${CI_CONTAINER} ${DOCKER_RUN_CMD}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ci/scripts/bootstrap_local_ci.sh(1 hunks)ci/scripts/common.sh(1 hunks)ci/scripts/run_ci_local.sh(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
ci/scripts/**/*.sh
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
CI shell/utility scripts must live under ci/scripts/
Files:
ci/scripts/common.shci/scripts/bootstrap_local_ci.shci/scripts/run_ci_local.sh
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,ini,json,ipynb,txt,rst}: Every file must start with the standard SPDX Apache-2.0 header; keep copyright years up‑to‑date
All source files must include the SPDX Apache‑2.0 header; do not bypass CI header checks
Files:
ci/scripts/common.shci/scripts/bootstrap_local_ci.shci/scripts/run_ci_local.sh
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
ci/scripts/common.shci/scripts/bootstrap_local_ci.shci/scripts/run_ci_local.sh
🪛 Shellcheck (0.10.0)
ci/scripts/bootstrap_local_ci.sh
[warning] 23-23: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (2)
ci/scripts/common.sh (1)
183-186: Extra LFS logging is good.The added log lines before fetch/pull improve diagnosability without changing behavior. LGTM.
ci/scripts/run_ci_local.sh (1)
47-51: Approve: HTTPS fallback when SSH agent missing — manual verification requiredLGTM; sandbox couldn't run the verification because SSH_AUTH_SOCK is unset (error: "/bin/bash: line 5: SSH_AUTH_SOCK: ensure your agent is running and loaded").
Run the supplied verification script locally: (1) with SSH agent set — expect no prompts and LFS objects fetched; (2) with SSH_AUTH_SOCK unset — expect warning and HTTPS fallback; LFS may fail if the repo/LFS requires auth. Report results.
|
/merge |
2 participants
Description
Closes #791
By Submitting this PR I confirm:
Summary by CodeRabbit