[fournos_launcher] Allow deploying clusterless jobs - #123
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 Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe PR adds clusterless and Fournos environment directives, permits submission without a cluster, introduces a launcher command and Tekton pipeline, and makes kubeconfig setup conditional when the secret is unavailable. ChangesClusterless launcher workflow
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as Fournos submit CLI
participant Submit as submit_job
participant Tekton as forge-launcher Pipeline
participant Forge as forge-step Task
CLI->>Submit: Provide clusterless or Fournos configuration
Submit->>Tekton: Start launcher pipeline
Tekton->>Forge: Run preflight and launcher tasks
Forge->>Forge: Select kubeconfig or clusterless mode
Tekton->>Forge: Export artifacts in finally
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
fournos/gitops/base/workflows/task-forge-step.yaml (1)
16-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlaceholder string duplicated between param default and script logic.
The value
"fournos-clusterless-placeholder"appears as the param default here and is hardcoded again in the script at lines 81 and 87. If one is updated without the other, the conditional logic silently breaks. Consider adding a comment cross-referencing the locations, or deriving the comparison from a single env var.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fournos/gitops/base/workflows/task-forge-step.yaml` around lines 16 - 17, The kubeconfig placeholder is duplicated between the parameter default and script logic. Update the workflow’s kubeconfig handling to derive the comparisons from the parameter/environment value or otherwise centralize the placeholder, ensuring the default and checks cannot diverge; anchor the change around the kubeconfig parameter and the script’s placeholder checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@fournos/gitops/base/workflows/task-forge-step.yaml`:
- Around line 16-17: The kubeconfig placeholder is duplicated between the
parameter default and script logic. Update the workflow’s kubeconfig handling to
derive the comparisons from the parameter/environment value or otherwise
centralize the placeholder, ensuring the default and checks cannot diverge;
anchor the change around the kubeconfig parameter and the script’s placeholder
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d09354f8-7089-40b9-8a99-611496cb8534
📒 Files selected for processing (2)
fournos/gitops/base/workflows/task-forge-step.yamlprojects/skeleton/orchestration/ci.py
🟢 Execution of
|
|
/test fournos skeleton |
🔴 Submission of
|
|
/test fournos skeleton |
🔴 Submission of
|
|
/test fournos skeleton |
🔴 Submission of
|
|
/test fournos skeleton |
🔴 Submission of
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
projects/fournos_launcher/toolbox/submit_and_wait/main.py (1)
49-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider relaxing the
cluster_nametype hint for clusterless mode.
cluster_name: stris a required positional parameter with no default, but in clusterless mode the caller (e.g.,submit.py) may passNonewhencluster.nameis absent from config. The type hint should reflect this:str | None = Noneorstr = "".♻️ Suggested type hint update
def run( - cluster_name: str, + cluster_name: str | None = None, project: str, *, args: list = None,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py` around lines 49 - 66, The run function’s cluster_name annotation requires a string even though clusterless callers may provide no cluster name. Update run’s cluster_name parameter to accept None and default appropriately, preserving existing behavior for configured cluster names and clusterless execution.projects/fournos_launcher/orchestration/cli.py (1)
60-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEliminate redundant
get_config("fournos.job.clusterless")call.
clusterless_modeis fetched at line 60 inside theelseblock and then unconditionally re-fetched at line 68. When theelsebranch is taken, this produces a duplicateget_configcall (including duplicate log output). Hoist the fetch above the validation so it's only called once.♻️ Proposed refactor
if cluster: config.project.set_config("cluster.name", cluster) else: cluster = config.project.get_config("cluster.name") - clusterless_mode = config.project.get_config("fournos.job.clusterless") - if not cluster and not clusterless_mode: + clusterless_mode = config.project.get_config("fournos.job.clusterless") + + if not cluster and not clusterless_mode: raise ValueError( "--cluster or cluster.name is mandatory (unless clusterless mode is enabled)" ) - # Validate clusterless and exclusive modes are not both enabled - clusterless_mode = config.project.get_config("fournos.job.clusterless") exclusive_mode = config.project.get_config("fournos.job.exclusive") if clusterless_mode and exclusive_mode: raise ValueError( "Clusterless mode and exclusive mode cannot both be enabled - use /clusterless (sets exclusive=false) or /exclusive false" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/fournos_launcher/orchestration/cli.py` around lines 60 - 78, Hoist the clusterless_mode assignment before the cluster/clusterless validation in the surrounding CLI function, then reuse that variable for both the mandatory-cluster check and the clusterless/exclusive conflict check. Remove the inner else-block fetch and the later duplicate get_config call, preserving the existing validation and logging behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@projects/fournos_launcher/orchestration/cli.py`:
- Around line 60-78: Hoist the clusterless_mode assignment before the
cluster/clusterless validation in the surrounding CLI function, then reuse that
variable for both the mandatory-cluster check and the clusterless/exclusive
conflict check. Remove the inner else-block fetch and the later duplicate
get_config call, preserving the existing validation and logging behavior.
In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py`:
- Around line 49-66: The run function’s cluster_name annotation requires a
string even though clusterless callers may provide no cluster name. Update run’s
cluster_name parameter to accept None and default appropriately, preserving
existing behavior for configured cluster names and clusterless execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62dfd778-ebe6-431d-b18e-bcac3cb3ab54
📒 Files selected for processing (6)
projects/fournos_launcher/orchestration/cli.pyprojects/fournos_launcher/orchestration/config.yamlprojects/fournos_launcher/orchestration/pr_args.pyprojects/fournos_launcher/orchestration/submit.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2
🚧 Files skipped from review as they are similar to previous changes (3)
- projects/fournos_launcher/orchestration/config.yaml
- projects/fournos_launcher/orchestration/submit.py
- projects/fournos_launcher/orchestration/pr_args.py
|
/test fournos skeleton |
🔴 Submission of
|
|
/test fournos skeleton |
🔴 Submission of
|
|
/test fournos skeleton |
🔴 Submission of
|
kpouget
changed the title
|
/test fournos skeleton |
🟢 Execution of
|
🟢 Submission of
|
🟢 Submission of
|
|
/test fournos skeleton |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos skeleton |
🔴 Submission of
|
|
/test fournos skeleton |
🟢 Execution of
|
🟢 Submission of
|
|
/test fournos skeleton |
🟢 Execution of
|
🟢 Submission of
|
|
tests passed in the WIP namespace, merging this 👍🏻 |
…voke a clusterless launcher
🟢 Execution of
|
1 participant
goes along with openshift-psap/fournos#99
Summary by CodeRabbit
launchercommand entry point and aforge-launcherTekton pipeline to run launcher tasks and always export artifacts (also expanded workflow pipeline resources)./clusterlessand/fournosdirectives, including validation and environment-based namespace targeting.