Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace class-based Jump CI runner with procedural Click commands; add a Fire-based toolbox entrypoint and a bin shim; introduce repo validation utilities (WIP and broken-symlink checks); update CI entrypoints, logging, Containerfile layout, PR-argument persistence; and add Ansible config, callback plugins, and inventory. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Runner
participant Repo as Repo.validate_no_wip
participant GH as GitHub API
participant Git as git subprocess
participant FS as Filesystem
CI->>Repo: invoke validate_no_wip()
Repo->>GH: GET /repos/:owner/:repo/pulls/:number
GH-->>Repo: PR JSON (title, etc.)
Repo->>Git: git log --pretty=%s parent_range...
Git-->>Repo: commit subject lines
Repo->>Repo: scan title + commits for "WIP"
alt WIP found
Repo-->>CI: exit 1 (failure)
else
Repo-->>CI: exit 0 (success)
end
CI->>Repo: invoke validate_no_broken_link()
Repo->>FS: os.walk('.') and os.readlink() for symlinks
FS-->>Repo: symlink entries and targets
alt any broken
Repo-->>CI: log broken links + exit 1
else
Repo-->>CI: log success + exit 0
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
/test llmd-jump-ci hello world |
|
/test llmd-jump-ci hello world |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 05 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 06 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
kpouget
force-pushed
the
jump-ci
branch
2 times, most recently
from
495fe3c to
00a8219
Compare
|
/test llmd-jump-ci hello world |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 13 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 06 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
/test llmd-jump-ci hello world |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 05 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 06 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
/test llmd-jump-ci hello world |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 05 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
projects/repo/toolbox/repo.py (1)
127-128: Anchor the symlink scan to the repository root.Walking
Path('.')makes the result depend on the caller's current directory.TOPSAIL_DIRis already available here and would make this check deterministic.📍 Make the scan deterministic
- check_directory(pathlib.Path('.')) + check_directory(TOPSAIL_DIR)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/repo/toolbox/repo.py` around lines 127 - 128, The symlink scan currently calls check_directory(pathlib.Path('.')) which depends on the process CWD; change the call to use the repo root variable TOPSAIL_DIR so the scan is deterministic. Update the invocation in the same scope (where logger.info and check_directory are called) to check_directory(TOPSAIL_DIR) and ensure TOPSAIL_DIR is imported/defined in this module if not already referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/core/ci_entrypoint/prepare_ci.py`:
- Around line 471-475: The fallback for the lightweight image should change the
chdir target from "/app" to the new repo location "/app/topsail": in
prepare_ci.py, update the branch that checks
os.environ.get("TOPSAIL_LIGHT_IMAGE") so that instead of calling
os.chdir("/app") it calls os.chdir("/app/topsail") (the branch that currently
uses the topsail_home variable and the os.chdir calls must be adjusted
accordingly so subsequent git/path checks run inside the repo).
- Around line 303-306: The current loop that prints all environment variables
(the sorted(os.environ.items()) loop) must be replaced to avoid leaking secrets:
stop dumping every key/value pair and instead either iterate only an explicit
allowlist of safe variable names (e.g., ENV_ALLOWLIST or SAFE_ENV_KEYS) and log
their values, or keep the existing iteration but redact values for
sensitive-looking keys (match patterns like "KEY", "SECRET", "TOKEN", "PASS",
"PASSWORD", "AWS", "GCP", "SSH", "CREDENTIAL" and replace values with
"<REDACTED>"). Update the code around logger.info("Environment variables:") and
the loop to use the allowlist or redaction policy so no credentials are printed.
- Around line 308-312: The logged git command shows the range "git show --quiet
--oneline {base_sha}..{pull_sha}" but the subprocess.run call uses only
f"{base_sha}", causing mismatch; update the subprocess invocation in
prepare_ci.py (the subprocess.run call that currently passes f"{base_sha}") to
use the same range string f"{base_sha}..{pull_sha}" so the command executed
matches the logger.info output (or alternatively change the logger.info to match
the single-sha call) and ensure the variables base_sha and pull_sha are
referenced consistently.
In `@projects/jump_ci/orchestration/ci.py`:
- Around line 103-107: The code is calling run_on_jump_ci("test_ci") (and
similarly in the other branch) which returns a runner function but never invokes
it; change the logic in test() and pre_cleanup() to capture the returned runner
(e.g., runner = run_on_jump_ci("test_ci")) and then call it (ret = runner()) and
pass that numeric return code to sys.exit(ret); update both occurrences (lines
around test() and the similar block at 116-121) to invoke the returned function
rather than treating the function object as the exit code.
- Around line 29-33: The --verbose flag is stored in the Click context but the
module-level log() function no longer uses it, so update log() to read the
current Click context (via click.get_current_context(silent=True)) and check
ctx.obj.get("verbose", False) (or ctx.obj["verbose"]) and use that to control
verbosity (e.g., suppress "debug" messages unless verbose is True, or emit extra
details when verbose is True); locate and modify the log() function to consult
the Click context/ctx.obj["verbose"] and adjust output behavior accordingly so
the flag actually affects logging.
- Around line 90-94: The code currently assigns the callable returned by
run_on_jump_ci("test_ci") to ret and exits with that object, skipping the
prepare phase; instead call the dedicated prepare step and execute its returned
runner: call run_on_jump_ci("prepare_ci") to get the runner (reference:
run_on_jump_ci and prepare_ci / jump_ci), invoke that runner to obtain the
integer exit code, then log using that exit code and pass it to sys.exit. Ensure
you replace the current assignment to ret with obtaining and calling the runner
so the prepare phase actually runs.
In `@projects/legacy/library/run.py`:
- Around line 111-112: The unconditional time.sleep(4000) in run() blocks hot
paths and must be removed; delete that call from the run() function and, if a
delay is required for a specific subprocess, replace it with a targeted,
configurable wait (e.g., retry/backoff logic or a parameterized timeout) scoped
to the specific operation (cluster detection, SSH probing, S3 export, or
artifact move) rather than sleeping unconditionally for all callers.
- Line 109: The debug log in run() currently accesses os.environ['PWD'] which
can raise KeyError in stripped environments; change the logging so it never
directly indexes the environment—use os.environ.get('PWD') with a fallback to
the existing cwd variable or call os.getcwd() (or wrap in try/except KeyError)
and log that resolved path instead, updating the logging.info line that
references cwd and os.environ['PWD'] accordingly.
In `@projects/repo/toolbox/repo.py`:
- Around line 47-49: The urllib.request.urlopen(pr_url) call can block
indefinitely; modify the call to pass an explicit timeout (e.g., timeout=5) so
the GitHub API request fails fast on stalls. Locate the code that opens pr_url
(the with urllib.request.urlopen(pr_url) as response block that assigns pr_data)
and add the timeout argument to urlopen; ensure any existing exception handling
around this block will catch the resulting timeout exception (URLError/timeout)
so the validation job can fail or retry appropriately.
---
Nitpick comments:
In `@projects/repo/toolbox/repo.py`:
- Around line 127-128: The symlink scan currently calls
check_directory(pathlib.Path('.')) which depends on the process CWD; change the
call to use the repo root variable TOPSAIL_DIR so the scan is deterministic.
Update the invocation in the same scope (where logger.info and check_directory
are called) to check_directory(TOPSAIL_DIR) and ensure TOPSAIL_DIR is
imported/defined in this module if not already referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfdc8f27-fc86-415c-a72e-618da7dfc291
📒 Files selected for processing (8)
.github/workflows/check_consistency.ymlbin/run_toolbox.pyprojects/core/ci_entrypoint/prepare_ci.pyprojects/core/image/Containerfile.lightweightprojects/jump_ci/orchestration/ci.pyprojects/legacy/library/run.pyprojects/legacy/library/run_toolbox.pyprojects/repo/toolbox/repo.py
| # Show all environment variables for debugging | ||
| logger.info("Environment variables:") | ||
| for key, value in sorted(os.environ.items()): | ||
| print(f"export {key}={value}") |
There was a problem hiding this comment.
Don't dump the full CI environment into logs.
This prints every environment variable, including credentials injected by CI. Add an allowlist or redact sensitive keys before logging.
🛡️ Safer debug output
- logger.info("Environment variables:")
- for key, value in sorted(os.environ.items()):
- print(f"export {key}={value}")
+ logger.info("Environment variables:")
+ sensitive_markers = ("TOKEN", "SECRET", "PASSWORD", "KEY", "COOKIE")
+ for key, value in sorted(os.environ.items()):
+ safe_value = "***" if any(marker in key.upper() for marker in sensitive_markers) else value
+ print(f"export {key}={safe_value}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/ci_entrypoint/prepare_ci.py` around lines 303 - 306, The
current loop that prints all environment variables (the
sorted(os.environ.items()) loop) must be replaced to avoid leaking secrets: stop
dumping every key/value pair and instead either iterate only an explicit
allowlist of safe variable names (e.g., ENV_ALLOWLIST or SAFE_ENV_KEYS) and log
their values, or keep the existing iteration but redact values for
sensitive-looking keys (match patterns like "KEY", "SECRET", "TOKEN", "PASS",
"PASSWORD", "AWS", "GCP", "SSH", "CREDENTIAL" and replace values with
"<REDACTED>"). Update the code around logger.info("Environment variables:") and
the loop to use the allowlist or redaction policy so no credentials are printed.
| def log(message: str, level: str = "info"): | ||
| """Log message with project prefix.""" | ||
| project_name = "jump ci" | ||
| icon = {"info": "ℹ️", "success": "✅", "error": "❌", "warning": "⚠️"}.get(level, "ℹ️") | ||
| click.echo(f"{icon} [{project_name}] {message}") |
There was a problem hiding this comment.
--verbose no longer changes anything in this entrypoint.
The refactor still stores verbose in ctx.obj, but the new module-level log() never consults it and nothing else in this file consumes ctx.obj["verbose"]. That makes the flag misleading for callers unless it is wired back into logging or removed.
Also applies to: 39-42
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 32-32: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
[warning] 32-32: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/jump_ci/orchestration/ci.py` around lines 29 - 33, The --verbose
flag is stored in the Click context but the module-level log() function no
longer uses it, so update log() to read the current Click context (via
click.get_current_context(silent=True)) and check ctx.obj.get("verbose", False)
(or ctx.obj["verbose"]) and use that to control verbosity (e.g., suppress
"debug" messages unless verbose is True, or emit extra details when verbose is
True); locate and modify the log() function to consult the Click
context/ctx.obj["verbose"] and adjust output behavior accordingly so the flag
actually affects logging.
| log("Starting test phase...") | ||
| try: | ||
| ret = run_on_jump_ci("test_ci") | ||
| log("Jump CI environment test completed", "success" if not ret else "error") | ||
| sys.exit(ret) |
There was a problem hiding this comment.
Call the generated runner in test() and pre_cleanup().
run_on_jump_ci() is a factory, not the execution step itself. These branches currently construct a function, treat that function object as the return code, and then sys.exit(ret), so neither command actually runs its Jump CI step.
🐛 Proposed fix
- ret = run_on_jump_ci("test_ci")
- log("Jump CI environment test completed", "success" if not ret else "error")
- sys.exit(ret)
+ run_on_jump_ci("test_ci")()
+ log("Jump CI environment test completed", "success")
+ sys.exit(0)- ret = run_on_jump_ci("pre_cleanup_ci")
- log("Cleanup phase completed!", "success" if not ret else "error")
- sys.exit(ret)
+ run_on_jump_ci("pre_cleanup_ci")()
+ log("Cleanup phase completed!", "success")
+ sys.exit(0)Also applies to: 116-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/jump_ci/orchestration/ci.py` around lines 103 - 107, The code is
calling run_on_jump_ci("test_ci") (and similarly in the other branch) which
returns a runner function but never invokes it; change the logic in test() and
pre_cleanup() to capture the returned runner (e.g., runner =
run_on_jump_ci("test_ci")) and then call it (ret = runner()) and pass that
numeric return code to sys.exit(ret); update both occurrences (lines around
test() and the similar block at 116-121) to invoke the returned function rather
than treating the function object as the exit code.
| if log_command: | ||
| logging.info(f"run: {command}") | ||
|
|
||
| logging.info(f"from: {cwd} or {os.environ['PWD']}") |
There was a problem hiding this comment.
Don't let the debug log depend on PWD being exported.
os.environ['PWD'] can raise before the subprocess is even spawned. Falling back to cwd/os.getcwd() keeps run() safe in stripped environments.
🧭 Safer cwd logging
- logging.info(f"from: {cwd} or {os.environ['PWD']}")
+ logging.info(f"from: {cwd or os.environ.get('PWD') or os.getcwd()}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logging.info(f"from: {cwd} or {os.environ['PWD']}") | |
| logging.info(f"from: {cwd or os.environ.get('PWD') or os.getcwd()}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/legacy/library/run.py` at line 109, The debug log in run() currently
accesses os.environ['PWD'] which can raise KeyError in stripped environments;
change the logging so it never directly indexes the environment—use
os.environ.get('PWD') with a fallback to the existing cwd variable or call
os.getcwd() (or wrap in try/except KeyError) and log that resolved path instead,
updating the logging.info line that references cwd and os.environ['PWD']
accordingly.
| import time | ||
| time.sleep(4000) |
There was a problem hiding this comment.
Remove the unconditional 4000-second sleep from run().
run() sits on hot paths like cluster detection, SSH probing, S3 export, and jump-ci artifact moves. Sleeping ~66 minutes before every subprocess will stall or timeout almost every workflow.
🚫 Remove the blocker
- import time
- time.sleep(4000)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import time | |
| time.sleep(4000) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/legacy/library/run.py` around lines 111 - 112, The unconditional
time.sleep(4000) in run() blocks hot paths and must be removed; delete that call
from the run() function and, if a delay is required for a specific subprocess,
replace it with a targeted, configurable wait (e.g., retry/backoff logic or a
parameterized timeout) scoped to the specific operation (cluster detection, SSH
probing, S3 export, or artifact move) rather than sleeping unconditionally for
all callers.
| # Fetch PR data from GitHub API | ||
| with urllib.request.urlopen(pr_url) as response: | ||
| pr_data = json.loads(response.read().decode()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and locate the file
find . -name "repo.py" -path "*/toolbox/*" | head -20Repository: openshift-psap/topsail-ng
Length of output: 100
🏁 Script executed:
# Read the relevant lines from the file
cat -n projects/repo/toolbox/repo.py | sed -n '40,60p'Repository: openshift-psap/topsail-ng
Length of output: 1118
🏁 Script executed:
# Search for timeout patterns in the repository
rg "timeout" --type py -i -B 2 -A 2 | head -50Repository: openshift-psap/topsail-ng
Length of output: 3808
Add a timeout to the GitHub API request to prevent indefinite hangs.
The urllib.request.urlopen() call on line 48 blocks without a timeout, allowing a transient GitHub stall to hang the entire validation job. The codebase already uses timeouts for similar API calls (e.g., 5 seconds in projects/jump_ci/testing/utils_gethostname.py).
⏱️ Bound the external call
- with urllib.request.urlopen(pr_url) as response:
+ with urllib.request.urlopen(pr_url, timeout=10) as response:
pr_data = json.loads(response.read().decode())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Fetch PR data from GitHub API | |
| with urllib.request.urlopen(pr_url) as response: | |
| pr_data = json.loads(response.read().decode()) | |
| # Fetch PR data from GitHub API | |
| with urllib.request.urlopen(pr_url, timeout=10) as response: | |
| pr_data = json.loads(response.read().decode()) |
🧰 Tools
🪛 Ruff (0.15.6)
[error] 48-48: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/repo/toolbox/repo.py` around lines 47 - 49, The
urllib.request.urlopen(pr_url) call can block indefinitely; modify the call to
pass an explicit timeout (e.g., timeout=5) so the GitHub API request fails fast
on stalls. Locate the code that opens pr_url (the with
urllib.request.urlopen(pr_url) as response block that assigns pr_data) and add
the timeout argument to urlopen; ensure any existing exception handling around
this block will catch the resulting timeout exception (URLError/timeout) so the
validation job can fail or retry appropriately.
|
/test llmd-jump-ci hello world |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 06 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 05 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
1 similar comment
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 05 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 06 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
🔴 Test of 'jump_ci ci' failed after 00 hours 00 minutes 05 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
kpouget
force-pushed
the
jump-ci
branch
2 times, most recently
from
6191b46 to
3b06d91
Compare
|
/test llmd-jump-ci hello world |
|
🔴 Test of 'jump_ci ci' succeeded after 00 hours 01 minutes 58 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
…e it work with TOPSAIL-ng
…e the KUBECONFIG optional
…rectly pass the user ID
|
🟢 Test of 'jump_ci test' succeeded after 00 hours 01 minutes 06 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
1 participant
Summary by CodeRabbit
New Features
Chores