Replace individual FORGE params with FJOB_NAME + FOURNOS_NAMESPACE#72
Merged
avasilevskii merged 4 commits intoopenshift-psap:mainfrom Apr 29, 2026
Merged
Conversation
Signed-off-by: avasilev <avasilev@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR changes how FORGE receives job configuration: the operator and Tekton pipelines now pass only Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant API as K8s API
participant Controller as Fournos Operator
participant Tekton as Tekton (PipelineRun)
participant Resolve as Resolve Job / FORGE
Operator->>Controller: create FournosJob CR (spec + forge/env)
Controller->>Tekton: create PipelineRun (params: fjob-name, fournos-namespace, kubeconfig-secret)
Tekton->>Resolve: start resolve task / container (env: FJOB_NAME, FOURNOS_NAMESPACE)
Resolve->>API: GET FournosJob/<FJOB_NAME> in <FOURNOS_NAMESPACE>
API-->>Resolve: FournosJob YAML
Resolve->>Resolve: extract .spec.forge, .spec.env, etc. (yq)
Resolve->>FORGE: invoke bin/run_ci with derived FORGE_PROJECT / env / artifacts
Resolve-->>Tekton: task completes (status)
Tekton-->>Controller: PipelineRun status updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 2
🧹 Nitpick comments (1)
fournos/core/resolve.py (1)
71-79: Fail fast when required resolve env placeholders are missing.This loop silently skips absent keys in the job template. A template drift will fail later in-container instead of at job creation time.
♻️ Proposed hardening
env_values = { "FJOB_NAME": name, "FOURNOS_NAMESPACE": settings.namespace, } + required = set(env_values) + present = {e.get("name") for e in container.get("env", [])} + missing = required - present + if missing: + raise ValueError( + f"Resolve job template is missing required env entries: {sorted(missing)}" + ) + for env_var in container["env"]: if env_var["name"] not in env_values: continue env_var["value"] = env_values[env_var["name"]]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fournos/core/resolve.py` around lines 71 - 79, The loop that patches container["env"] using env_values currently skips missing keys, causing template drift to surface later; change it to fail fast by verifying that each required key from env_values is present in the container's env list and raising a clear exception (e.g., ValueError) that includes the job name and missing env var(s). Specifically, iterate over keys of env_values (or build a set of expected names) and for each expected name ensure there's a matching dict in container["env"]; if not found, raise an error referencing env_values, container["env"], name and settings.namespace so the caller sees which placeholder is missing. Ensure you still set env_var["value"] for matches as the current code does.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/forge/workflows/tasks.yaml`:
- Line 52: The task uses an undefined variable FOURNOS_NAME in the oc get
command; replace it with the correctly defined FJOB_NAME (and likewise replace
any other occurrences such as the one around line 81) so the command becomes oc
get "fjob/$FJOB_NAME" -n "$FOURNOS_NAMESPACE" ...; update both occurrences to
consistently use FJOB_NAME (leave FOURNOS_NAMESPACE as-is if that variable is
defined) to avoid set -o nounset failures and ensure the step reaches run_ci.
- Around line 20-25: The task is using the mounted workload KUBECONFIG when
running the forge-step which causes `oc get fjob/...` to query the target
cluster instead of the hub; update the task so the forge-step fetches the
FournosJob from the hub context (same approach used in resolve_job.yaml).
Concretely, remove or stop setting KUBECONFIG to "$(params.kubeconfig-secret)"
for the step that runs `oc get fjob/...` (refer to the KUBECONFIG env and the
forge-step that executes `oc get fjob`), or explicitly point KUBECONFIG there to
the hub kubeconfig secret instead of the workload mount; ensure FJOB_NAME and
FOURNOS_NAMESPACE are still passed through and that only the hub cluster context
is used for the FournosJob lookup.
---
Nitpick comments:
In `@fournos/core/resolve.py`:
- Around line 71-79: The loop that patches container["env"] using env_values
currently skips missing keys, causing template drift to surface later; change it
to fail fast by verifying that each required key from env_values is present in
the container's env list and raising a clear exception (e.g., ValueError) that
includes the job name and missing env var(s). Specifically, iterate over keys of
env_values (or build a set of expected names) and for each expected name ensure
there's a matching dict in container["env"]; if not found, raise an error
referencing env_values, container["env"], name and settings.namespace so the
caller sees which placeholder is missing. Ensure you still set env_var["value"]
for matches as the current code does.
🪄 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: 36921227-4c14-404b-b481-a656038fa6c4
📒 Files selected for processing (20)
Fournos_Design_Document.mdREADME.mdconfig/forge/resolve_job.yamlconfig/forge/workflows/pipeline-full.yamlconfig/forge/workflows/pipeline-test-only.yamlconfig/forge/workflows/tasks.yamlconfig/fournos-validation/workflows/pipeline-validate-only.yamlconfig/fournos-validation/workflows/tasks.yamldev/mock-pipelines/pipeline-full.yamldev/mock-pipelines/pipeline-run-only.yamldev/mock-pipelines/tasks.yamldev/mock-resolve/resolve.shdev/mock-resolve/resolve_job.yamlfournos/core/resolve.pyfournos/core/tekton.pyfournos/handlers/execution.pyfournos/handlers/resolving.pytests/conftest.pytests/test_scheduling.pytests/test_secret_refs.py
💤 Files with no reviewable changes (5)
- config/fournos-validation/workflows/tasks.yaml
- fournos/handlers/resolving.py
- tests/test_scheduling.py
- tests/test_secret_refs.py
- fournos/handlers/execution.py
Signed-off-by: avasilev <avasilev@redhat.com>
Signed-off-by: avasilev <avasilev@redhat.com>
kpouget
reviewed
Apr 29, 2026
Collaborator
|
thanks, looks good! /lgtm |
Signed-off-by: avasilev <avasilev@redhat.com>
|
New changes are detected. LGTM label has been removed. |
|
[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 |
Collaborator
Author
|
/test deploy-fournos-wip |
|
🟢 Test of 'fournos_deploy --project-source' succeeded after 00 hours 10 minutes 54 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
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
2 participants
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.
Instead of extracting
forge-project,forge-config, andenvfrom theFournosJobspec and passing them as separate PipelineRun/resolve Job params, Fournos now passes onlyFJOB_NAMEandFOURNOS_NAMESPACE. FORGE looks up the fullFournosJobspec via the K8s API, giving it access to all configuration in one go.This also drops
job-name,gpu-count, andsecret-refsas pipeline params since FORGE can read those from the spec directly if required. The only params remaining on the PipelineRun arefjob-name,fournos-namespace, andkubeconfig-secret.Closes #61
Summary by CodeRabbit