[Fournos testing] Continue the foreign testing integration#32
Merged
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 |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 56 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds per-container and init-container log capture for build pods, adjusts CI entrypoint signal/argument handling and error messages, and makes foreign-testing launch parameters configurable with CLI and submit-time project-path validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/core/ci_entrypoint/run_ci.py (1)
424-434:⚠️ Potential issue | 🟡 MinorKeep the existence check and chmod hint on the absolute path.
Line 424 turns
script_pathinto a repo-relative path, but Line 426 still uses it for.exists()and Line 434 prints it in the remediation command. When this entrypoint runs outsideFORGE_HOME, an existing non-executable script is misclassified as missing, and the suggestedchmodpath is wrong. Keep an absolute path for filesystem checks and derive a separate relative path only for display.Suggested fix
- script_path = project_dir.relative_to(FORGE_HOME) / "orchestration" / f"{operation}.py" + abs_script_path = project_dir / "orchestration" / f"{operation}.py" + script_path = abs_script_path.relative_to(FORGE_HOME) - if script_path.exists(): + if abs_script_path.exists(): click.echo( click.style( f"❌ ERROR: CI script '{script_path} 'exists but is not executable for project '{project}' operation '{operation}'.", fg='red' ), err=True ) - click.echo(f"💡 Fix with: chmod +x {script_path}") + click.echo(f"💡 Fix with: chmod +x {abs_script_path}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/ci_entrypoint/run_ci.py` around lines 424 - 434, The code makes script_path repo-relative then uses it for filesystem checks and the chmod hint, causing false negatives and wrong remediation when running outside FORGE_HOME; change to compute an absolute path for filesystem operations (e.g., abs_script_path = project_dir / "orchestration" / f"{operation}.py") and separately derive a display-relative path for messages (e.g., rel_script_path = abs_script_path.relative_to(FORGE_HOME) or similar). Use abs_script_path.exists() for the existence check and print the chmod hint with the absolute path, while using rel_script_path only in user-facing explanatory text.
🤖 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/cluster/toolbox/rebuild_image/main.py`:
- Around line 37-89: The _capture_all_container_logs helper expects a Kubernetes
Pod name but callers pass ctx.buildrun_name (a BuildRun name), so oc get pod
fails; before invoking _capture_all_container_logs, resolve the real pod by
reading the BuildRun.status to find the referenced TaskRun (follow
BuildRun.status.taskRunRef / ownerReferences or status fields) and then read
TaskRun.status.podName (or TaskRun.status.steps[]/podName) to get the actual pod
name; update the caller to fetch the TaskRun reference from the BuildRun (using
the buildrun name), read the TaskRun status.podName, and pass that podName into
_capture_all_container_logs instead of ctx.buildrun_name.
In `@projects/foreign_testing/orchestration/cli.py`:
- Around line 26-32: The Click option for --project-path currently passes a
string; update the click.option on the submit command to use
type=click.Path(..., path_type=pathlib.Path) so Click supplies a pathlib.Path to
submit(ctx, project_path) and to foreign_testing.submit (which calls .exists());
this ensures .exists() works and preserves existing validation/behavior.
---
Outside diff comments:
In `@projects/core/ci_entrypoint/run_ci.py`:
- Around line 424-434: The code makes script_path repo-relative then uses it for
filesystem checks and the chmod hint, causing false negatives and wrong
remediation when running outside FORGE_HOME; change to compute an absolute path
for filesystem operations (e.g., abs_script_path = project_dir / "orchestration"
/ f"{operation}.py") and separately derive a display-relative path for messages
(e.g., rel_script_path = abs_script_path.relative_to(FORGE_HOME) or similar).
Use abs_script_path.exists() for the existence check and print the chmod hint
with the absolute path, while using rel_script_path only in user-facing
explanatory text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a111fff-8ea2-4522-8971-1317634e6f36
📒 Files selected for processing (6)
projects/cluster/toolbox/build_image/main.pyprojects/cluster/toolbox/rebuild_image/main.pyprojects/core/ci_entrypoint/run_ci.pyprojects/foreign_testing/orchestration/ci.pyprojects/foreign_testing/orchestration/cli.pyprojects/foreign_testing/orchestration/foreign_testing.py
kpouget
force-pushed
the
fournos-testing
branch
2 times, most recently
from
cb23834 to
da458a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
projects/cluster/toolbox/build_image/main.py (1)
244-245:⚠️ Potential issue | 🟠 MajorResolve BuildRun → TaskRun → Pod before calling log capture.
Line 245 and Line 277 still pass a BuildRun name into a helper that runs
oc get pod <name>. This is the same unresolved issue from prior review: BuildRun name is not a guaranteed pod name, so container log capture can be skipped.Proposed fix
- _capture_all_container_logs(ctx.buildrun_name, args.namespace, args.artifact_dir) + pod_name = _resolve_buildrun_pod_name(ctx.buildrun_name, args.namespace) + if pod_name: + _capture_all_container_logs(pod_name, args.namespace, args.artifact_dir)- _capture_all_container_logs(build_run_name, args.namespace, args.artifact_dir) + pod_name = _resolve_buildrun_pod_name(build_run_name, args.namespace) + if pod_name: + _capture_all_container_logs(pod_name, args.namespace, args.artifact_dir)def _resolve_buildrun_pod_name(buildrun_name: str, namespace: str) -> str | None: br = shell.run( f"oc get buildruns.shipwright.io {buildrun_name} -n {namespace} -o json", check=False, log_stdout=False, ) if not br.success: logger.warning(f"Could not fetch BuildRun {buildrun_name}") return None info = json.loads(br.stdout) taskrun_name = info.get("status", {}).get("taskRunName") or info.get("status", {}).get( "executor", {} ).get("name") if not taskrun_name: logger.warning(f"Could not resolve TaskRun name from BuildRun {buildrun_name}") return None tr = shell.run( f"oc get taskrun {taskrun_name} -n {namespace} -o jsonpath='{{.status.podName}}'", check=False, log_stdout=False, ) if not tr.success or not tr.stdout.strip(): logger.warning(f"Could not resolve podName from TaskRun {taskrun_name}") return None return tr.stdout.strip()#!/bin/bash # Verify the name mapping on a live cluster NS=<namespace> BR=<buildrun-name> TR=$(oc get buildruns.shipwright.io "$BR" -n "$NS" -o jsonpath='{.status.taskRunName}') if [ -z "$TR" ]; then TR=$(oc get buildruns.shipwright.io "$BR" -n "$NS" -o jsonpath='{.status.executor.name}') fi POD=$(oc get taskrun "$TR" -n "$NS" -o jsonpath='{.status.podName}') echo "BuildRun: $BR" echo "TaskRun: $TR" echo "Pod: $POD"Also applies to: 276-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/cluster/toolbox/build_image/main.py` around lines 244 - 245, The BuildRun name is being passed directly into _capture_all_container_logs which calls `oc get pod <name>`; instead add a resolver function (e.g. _resolve_buildrun_pod_name(buildrun_name: str, namespace: str) -> str | None) that fetches the BuildRun JSON, extracts status.taskRunName or status.executor.name, then queries the TaskRun for status.podName, returning the pod name or None; call this resolver before invoking _capture_all_container_logs (and in the other place where BuildRun was passed), and if it returns None skip container log capture and log a warning. Ensure you use shell.run with check=False/log_stdout=False and json parsing to avoid raising on missing fields and reference the existing _capture_all_container_logs and the new _resolve_buildrun_pod_name symbols when making the change.
🤖 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/cluster/toolbox/build_image/main.py`:
- Around line 51-57: The debug messages claiming logs were "Captured" are
emitted unconditionally after calls to shell.run with check=False (e.g. the
shell.run(...) that uses pod_name, namespace, container_name and writes to
log_file), which can mislead if oc logs failed; change each such call to capture
the call's result (e.g. result = shell.run(..., check=False)) and only call
logger.debug("Captured logs...") when result indicates success (e.g.
result.returncode == 0) otherwise log an error or warning with the return code
and any stderr; apply the same pattern to the other block around lines 74-79 so
you only emit success debug logs when the command actually succeeded.
---
Duplicate comments:
In `@projects/cluster/toolbox/build_image/main.py`:
- Around line 244-245: The BuildRun name is being passed directly into
_capture_all_container_logs which calls `oc get pod <name>`; instead add a
resolver function (e.g. _resolve_buildrun_pod_name(buildrun_name: str,
namespace: str) -> str | None) that fetches the BuildRun JSON, extracts
status.taskRunName or status.executor.name, then queries the TaskRun for
status.podName, returning the pod name or None; call this resolver before
invoking _capture_all_container_logs (and in the other place where BuildRun was
passed), and if it returns None skip container log capture and log a warning.
Ensure you use shell.run with check=False/log_stdout=False and json parsing to
avoid raising on missing fields and reference the existing
_capture_all_container_logs and the new _resolve_buildrun_pod_name symbols when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbb52146-0172-4d1f-a684-047031837388
📒 Files selected for processing (7)
projects/cluster/toolbox/build_image/main.pyprojects/cluster/toolbox/rebuild_image/main.pyprojects/core/ci_entrypoint/run_ci.pyprojects/foreign_testing/orchestration/ci.pyprojects/foreign_testing/orchestration/cli.pyprojects/foreign_testing/orchestration/config.yamlprojects/foreign_testing/orchestration/foreign_testing.py
🚧 Files skipped from review as they are similar to previous changes (5)
- projects/foreign_testing/orchestration/ci.py
- projects/core/ci_entrypoint/run_ci.py
- projects/cluster/toolbox/rebuild_image/main.py
- projects/foreign_testing/orchestration/cli.py
- projects/foreign_testing/orchestration/foreign_testing.py
kpouget
force-pushed
the
fournos-testing
branch
from
da458a4 to
ca5ae3b
Compare
Contributor
Author
|
/test forge skeleton |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
projects/foreign_testing/orchestration/config.yaml (1)
5-11: Minor: inconsistent YAML indentation between sibling blocks.Children of
project_mappings:(line 5) use 8-space indentation (line 6), while children oflaunch:(line 7) use 6-space indentation (lines 8-11). Both are valid YAML, but the mixed depth is visually confusing. Consider normalizing to a single style (e.g., 6 spaces throughout) for readability.♻️ Proposed normalization
project_mappings: - fournos_deploy: tests/forge/deploy + fournos_deploy: tests/forge/deploy launch: project: fournos_deploy operation: ci args: - deploy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/foreign_testing/orchestration/config.yaml` around lines 5 - 11, The YAML uses inconsistent indentation between the mapping key project_mappings (children indented 8 spaces) and the launch block (children indented 6 spaces); normalize the file to use a single consistent indentation level (e.g., 6 spaces) for child entries under project_mappings and launch so keys like fournos_deploy, project, operation, and args all align visually and avoid confusion when editing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@projects/foreign_testing/orchestration/config.yaml`:
- Around line 5-11: The YAML uses inconsistent indentation between the mapping
key project_mappings (children indented 8 spaces) and the launch block (children
indented 6 spaces); normalize the file to use a single consistent indentation
level (e.g., 6 spaces) for child entries under project_mappings and launch so
keys like fournos_deploy, project, operation, and args all align visually and
avoid confusion when editing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d36b7905-528e-4cb6-97ea-9e92fd5c2a4b
📒 Files selected for processing (5)
projects/core/ci_entrypoint/run_ci.pyprojects/foreign_testing/orchestration/ci.pyprojects/foreign_testing/orchestration/cli.pyprojects/foreign_testing/orchestration/config.yamlprojects/foreign_testing/orchestration/foreign_testing.py
🚧 Files skipped from review as they are similar to previous changes (4)
- projects/foreign_testing/orchestration/ci.py
- projects/foreign_testing/orchestration/cli.py
- projects/core/ci_entrypoint/run_ci.py
- projects/foreign_testing/orchestration/foreign_testing.py
kpouget
force-pushed
the
fournos-testing
branch
from
ca5ae3b to
0391c04
Compare
Contributor
Author
|
/test fournos skeleton |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 11 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos skeleton |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 28 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
FOURNOS test passed in openshift-psap/fournos#30 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
submitgains optional --project-path to specify a project location.Bug Fixes
Behavior Changes