Introduce executionEngine in CRD#77
Merged
avasilevskii merged 7 commits intoopenshift-psap:mainfrom May 4, 2026
Merged
Conversation
Signed-off-by: avasilev <avasilev@redhat.com>
|
[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 |
|
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 (5)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughWalkthroughThe PR generalizes the Forge-specific resolve flow into a pluggable ChangesExecution Engine / Resolve flow
Sequence Diagram(s)sequenceDiagram
participant Operator
participant TektonAPI as Tekton Pipeline API
participant ResolveJob as K8s Job (Resolve)
participant API as Fournos API / CRD
Operator->>API: receives FournosJob (spec.pipeline, spec.executionEngine)
Operator->>TektonAPI: get_pipeline(name from spec.pipeline)
TektonAPI-->>Operator: Pipeline object (includes annotation)
Operator->>Operator: read annotation `fournos.dev/resolve-image`
Operator->>ResolveJob: create Job with container.image = annotation value
ResolveJob-->>Operator: patches FournosJob (hardware, secretRefs)
Operator->>API: update FournosJob status -> Resolved / proceed to scheduling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
79-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocumentation references outdated field paths.
The spec fields table still references
spec.forge.project,spec.forge.args, andspec.forge.configOverrides, but the CRD now usesspec.executionEngineSpec.project,spec.executionEngineSpec.args, andspec.executionEngineSpec.configOverrides.📝 Proposed fix
| Field | Required | Description | |---|---|---| -| `spec.forge.project` | yes | FORGE project path | -| `spec.forge.args` | yes | List of arguments passed to FORGE | -| `spec.forge.configOverrides` | no | Arbitrary YAML overrides passed to the test framework | +| `spec.executionEngine` | yes | Execution engine to use (e.g. "forge") | +| `spec.executionEngineSpec.resolveImage` | yes | Short image name for the resolve Job (e.g. "forge-core:main") | +| `spec.executionEngineSpec.project` | yes | FORGE project path | +| `spec.executionEngineSpec.args` | yes | List of arguments passed to FORGE | +| `spec.executionEngineSpec.configOverrides` | no | Arbitrary YAML overrides passed to the test framework |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 79 - 81, Update the README table to use the current CRD field paths: replace references to spec.forge.project, spec.forge.args, and spec.forge.configOverrides with spec.executionEngineSpec.project, spec.executionEngineSpec.args, and spec.executionEngineSpec.configOverrides respectively (ensure descriptions remain the same and YAML/markdown formatting stays valid).Fournos_Design_Document.md (1)
47-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocumentation references outdated field paths.
Similar to README.md, the spec table still references
spec.forge.project,spec.forge.args, andspec.forge.configOverrides. These should be updated to reflect the newspec.executionEngineSpec.*paths.📝 Proposed fix
| Field | Required | Description | | ---------------------------- | ------------ | ------------------------------------------------------------------------------------------------ | -| `spec.forge.project` | yes | FORGE project path | -| `spec.forge.args` | yes | List of arguments passed to FORGE | -| `spec.forge.configOverrides` | no | Arbitrary YAML overrides passed to the test framework | +| `spec.executionEngine` | yes | Execution engine to use (e.g. "forge") | +| `spec.executionEngineSpec.resolveImage` | yes | Short image name for the resolve Job (e.g. "forge-core:main") | +| `spec.executionEngineSpec.project` | yes | FORGE project path | +| `spec.executionEngineSpec.args` | yes | List of arguments passed to FORGE | +| `spec.executionEngineSpec.configOverrides` | no | Arbitrary YAML overrides passed to the test framework |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Fournos_Design_Document.md` around lines 47 - 49, Update the documentation table entries that reference deprecated fields `spec.forge.project`, `spec.forge.args`, and `spec.forge.configOverrides` to use the new `spec.executionEngineSpec.*` paths (e.g., `spec.executionEngineSpec.project`, `spec.executionEngineSpec.args`, `spec.executionEngineSpec.configOverrides`) and adjust their descriptions if needed to match the new naming; ensure any other mentions of `spec.forge.*` in Fournos_Design_Document.md are replaced with the corresponding `spec.executionEngineSpec.*` identifiers so the doc and README are consistent.fournos/handlers/resolving.py (1)
95-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMultiple stale "Forge" strings in user-facing messages contradict the PR objective.
The PR goal is explicitly to remove hardcoded Forge references, but the following strings still embed them:
Line Current string 3–4 (docstring) "launching a Forge resolve Job"95 "Resolving job requirements via Forge"119 "Forge resolution failed: …"135 (docstring) "Forge populates spec.hardware when absent"162 "spec.hardware not populated after Forge resolution"170 "(Forge may not have populated spec.hardware)"278 "Forge resolution complete"✏️ Suggested replacements
-Covers the Resolving phase: launching a Forge resolve Job that patches +Covers the Resolving phase: launching a resolve Job that patches- patch.status["message"] = "Resolving job requirements via Forge" + patch.status["message"] = "Resolving job requirements via execution engine"- f"Forge resolution failed: {message}", + f"Resolution failed: {message}",- Forge populates ``spec.hardware`` when absent. The GPU type is + The execution engine populates ``spec.hardware`` when absent. The GPU type is- "No hardware requirements: spec.hardware not populated " - "after Forge resolution", + "No hardware requirements: spec.hardware not populated " + "after resolution",- "Workload will only request cluster-slot resources " - "(Forge may not have populated spec.hardware)", + "Workload will only request cluster-slot resources " + "(execution engine may not have populated spec.hardware)",- "Forge resolution complete", + "Resolution complete",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fournos/handlers/resolving.py` at line 95, Several user-facing strings and docstrings still hardcode "Forge" contrary to the PR goal; replace those occurrences with a neutral term (e.g., "resolver", "resolution service", or "resolution") so messages are generic. Specifically update the docstrings that mention "launching a Forge resolve Job" and "Forge populates spec.hardware when absent", change patch.status["message"] value ("Resolving job requirements via Forge"), the error text thrown/logged ("Forge resolution failed: …" and "spec.hardware not populated after Forge resolution" and "(Forge may not have populated spec.hardware)"), and the completion message ("Forge resolution complete") to use the neutral wording; ensure any tests or references that rely on the old phrases are updated accordingly.
🧹 Nitpick comments (3)
manifests/ocpci-sa/role.yaml (1)
7-9: ⚡ Quick winSplit
fournosjobs/statusinto its own rule with only applicable verbs.
fournosjobs/statusis bundled with the parent resource, inheriting verbs (create,delete,list,watch) that the Kubernetes API server never honors on a/statussubresource. This is an over-broad RBAC grant that violates least-privilege — auditing tools and policy scanners will flag it, even though the extra verbs are inert at runtime.The operator's own role (
manifests/rbac/role_fournos.yaml) avoids this pattern, using onlyget,list,watch,patch,updatewithout mixing subresources and parent resources. Apply the same discipline here — determine whether the SA only reads status (get) or also writes it (patch/update), then split accordingly:🔒 Proposed fix — separate the status subresource rule
- - apiGroups: ["fournos.dev"] - resources: ["fournosjobs", "fournosjobs/status"] - verbs: ["create", "get", "list", "watch", "delete", "patch"] + - apiGroups: ["fournos.dev"] + resources: ["fournosjobs"] + verbs: ["create", "get", "list", "watch", "delete", "patch"] + - apiGroups: ["fournos.dev"] + resources: ["fournosjobs/status"] + verbs: ["get", "patch"] # add "update" if PUT-based status writes are neededThe same fix should be applied to
manifests/ocpci-sa/rbac-wip-ns.yamlLine 8.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@manifests/ocpci-sa/role.yaml` around lines 7 - 9, The RBAC rule currently bundles "fournosjobs" and the subresource "fournosjobs/status" together granting verbs like create/delete that don't apply to the /status subresource; split this into two separate rules: one rule for the parent resource "fournosjobs" with only the verbs the SA actually needs on the main resource (e.g., "create","get","list","watch","delete" if it needs them) and a separate rule for "fournosjobs/status" that grants only applicable status verbs (e.g., "get" and, if the SA must update status, "patch" and/or "update" — mirror the operator pattern of "get","list","watch","patch","update" for status if writes are needed). Apply the identical split/fixes to the other RBAC file that contains the same combined entry for "fournosjobs/status".manifests/crd.yaml (1)
86-89: 💤 Low valueConsider adding validation for
executionEnginevalues.The
executionEnginefield accepts any string, but currently only"forge"is a supported value. Consider adding anenumconstraint to catch typos early and provide better UX.This is optional since the execution engine validation could happen at runtime in the controller.
💡 Proposed schema constraint
executionEngine: type: string + enum: + - forge description: >- Execution engine to use for this job (e.g. "forge").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@manifests/crd.yaml` around lines 86 - 89, The CRD's executionEngine schema currently allows any string; restrict it by adding an enum constraint on the executionEngine property (the executionEngine field in the CRD schema) so only supported values (e.g., "forge") are accepted, and update the description to mention the allowed values; this will catch typos early and improve UX while the controller can still perform runtime validation for additional engines.fournos/core/resolve.py (1)
1-1: 💤 Low valueStale module docstring.
"Resolve client — manages Forge resolve K8s Jobs."still references Forge specifically.✏️ Suggested fix
-"""Resolve client — manages Forge resolve K8s Jobs.""" +"""Resolve client — manages execution-engine resolve K8s Jobs."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fournos/core/resolve.py` at line 1, The module-level docstring in resolve.py is stale (mentions "Forge"); update the top-of-file docstring to a generic, accurate description such as "Resolve client — manages resolve Kubernetes Jobs" or "Resolve client — manages K8s Jobs for content resolution", removing any Forge-specific wording so the docstring accurately describes the Resolve client module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fournos/handlers/resolving.py`:
- Around line 62-66: The code is calling .format() on the user-supplied
resolveImageRegistry and concatenating it directly with
engine_spec["resolveImage"], which causes KeyError on stray braces and malformed
image paths when the registry lacks a trailing slash. Replace the
.format(namespace=...) call with a targeted replace: substitute the literal
"{namespace}" token with settings.namespace (i.e. registry =
engine_spec.get("resolveImageRegistry",
settings.resolve_image_registry).replace("{namespace}", settings.namespace)).
Then normalize the separator before building resolve_image: ensure registry ends
with a single '/' (or that resolveImage does not start with '/'), adding a '/'
only when needed, and then set resolve_image = registry +
engine_spec["resolveImage"] so the final image path is well-formed.
---
Outside diff comments:
In `@Fournos_Design_Document.md`:
- Around line 47-49: Update the documentation table entries that reference
deprecated fields `spec.forge.project`, `spec.forge.args`, and
`spec.forge.configOverrides` to use the new `spec.executionEngineSpec.*` paths
(e.g., `spec.executionEngineSpec.project`, `spec.executionEngineSpec.args`,
`spec.executionEngineSpec.configOverrides`) and adjust their descriptions if
needed to match the new naming; ensure any other mentions of `spec.forge.*` in
Fournos_Design_Document.md are replaced with the corresponding
`spec.executionEngineSpec.*` identifiers so the doc and README are consistent.
In `@fournos/handlers/resolving.py`:
- Line 95: Several user-facing strings and docstrings still hardcode "Forge"
contrary to the PR goal; replace those occurrences with a neutral term (e.g.,
"resolver", "resolution service", or "resolution") so messages are generic.
Specifically update the docstrings that mention "launching a Forge resolve Job"
and "Forge populates spec.hardware when absent", change patch.status["message"]
value ("Resolving job requirements via Forge"), the error text thrown/logged
("Forge resolution failed: …" and "spec.hardware not populated after Forge
resolution" and "(Forge may not have populated spec.hardware)"), and the
completion message ("Forge resolution complete") to use the neutral wording;
ensure any tests or references that rely on the old phrases are updated
accordingly.
In `@README.md`:
- Around line 79-81: Update the README table to use the current CRD field paths:
replace references to spec.forge.project, spec.forge.args, and
spec.forge.configOverrides with spec.executionEngineSpec.project,
spec.executionEngineSpec.args, and spec.executionEngineSpec.configOverrides
respectively (ensure descriptions remain the same and YAML/markdown formatting
stays valid).
---
Nitpick comments:
In `@fournos/core/resolve.py`:
- Line 1: The module-level docstring in resolve.py is stale (mentions "Forge");
update the top-of-file docstring to a generic, accurate description such as
"Resolve client — manages resolve Kubernetes Jobs" or "Resolve client — manages
K8s Jobs for content resolution", removing any Forge-specific wording so the
docstring accurately describes the Resolve client module.
In `@manifests/crd.yaml`:
- Around line 86-89: The CRD's executionEngine schema currently allows any
string; restrict it by adding an enum constraint on the executionEngine property
(the executionEngine field in the CRD schema) so only supported values (e.g.,
"forge") are accepted, and update the description to mention the allowed values;
this will catch typos early and improve UX while the controller can still
perform runtime validation for additional engines.
In `@manifests/ocpci-sa/role.yaml`:
- Around line 7-9: The RBAC rule currently bundles "fournosjobs" and the
subresource "fournosjobs/status" together granting verbs like create/delete that
don't apply to the /status subresource; split this into two separate rules: one
rule for the parent resource "fournosjobs" with only the verbs the SA actually
needs on the main resource (e.g., "create","get","list","watch","delete" if it
needs them) and a separate rule for "fournosjobs/status" that grants only
applicable status verbs (e.g., "get" and, if the SA must update status, "patch"
and/or "update" — mirror the operator pattern of
"get","list","watch","patch","update" for status if writes are needed). Apply
the identical split/fixes to the other RBAC file that contains the same combined
entry for "fournosjobs/status".
🪄 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: 252baf6c-26c9-46ea-85d9-95081813d4b2
📒 Files selected for processing (23)
Fournos_Design_Document.mdMakefileREADME.mdconfig/forge/resolve_job.yamlconfig/forge/samples/job-full.yamlconfig/forge/samples/job-test-only.yamlconfig/forge/workflows/tasks.yamlconfig/fournos-validation/samples/test-connectivity-job.yamldev/job-secret-demo.yamldev/sample-job.yamlfournos/core/resolve.pyfournos/handlers/resolving.pyfournos/settings.pymanifests/crd.yamlmanifests/ocpci-sa/rbac-wip-ns.yamlmanifests/ocpci-sa/role.yamltests/test_exclusive.pytests/test_lifecycle.pytests/test_resolving.pytests/test_scheduling.pytests/test_secret_refs.pytests/test_shutdown.pytests/test_validation.py
Signed-off-by: avasilev <avasilev@redhat.com>
Signed-off-by: avasilev <avasilev@redhat.com>
…ve image Signed-off-by: avasilev <avasilev@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@manifests/crd.yaml`:
- Around line 85-91: The CRD currently allows any or multiple engines under
spec.executionEngine which causes runtime failures when the resolver expects a
single "forge" engine; update the executionEngine schema to require exactly the
forge key and validate its contents by replacing
x-kubernetes-preserve-unknown-fields: true with explicit properties: add a
required property "forge" of type object, define inside it the required field
"project" (and its type, e.g. string), and set additionalProperties: false on
spec.executionEngine so no other engine keys are permitted—this enforces
single-engine cardinality and ensures .spec.executionEngine.forge.project is
present.
🪄 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: 8224d6b3-2205-4050-93d7-575ba5bdf443
📒 Files selected for processing (28)
Fournos_Design_Document.mdMakefileREADME.mdconfig/forge/resolve_job.yamlconfig/forge/samples/job-full.yamlconfig/forge/samples/job-test-only.yamlconfig/forge/workflows/pipeline-full.yamlconfig/forge/workflows/pipeline-test-only.yamlconfig/forge/workflows/tasks.yamlconfig/fournos-validation/samples/test-connectivity-job.yamlconfig/fournos-validation/workflows/pipeline-validate-only.yamldev/job-secret-demo.yamldev/mock-pipelines/pipeline-full.yamldev/mock-pipelines/pipeline-run-only.yamldev/mock-pipelines/tasks.yamldev/sample-job.yamlfournos/core/tekton.pyfournos/handlers/resolving.pyfournos/settings.pymanifests/crd.yamlmanifests/rbac/role_fournos.yamltests/test_exclusive.pytests/test_lifecycle.pytests/test_resolving.pytests/test_scheduling.pytests/test_secret_refs.pytests/test_shutdown.pytests/test_validation.py
💤 Files with no reviewable changes (2)
- Makefile
- fournos/settings.py
✅ Files skipped from review due to trivial changes (3)
- config/forge/workflows/pipeline-test-only.yaml
- config/forge/workflows/pipeline-full.yaml
- dev/mock-pipelines/pipeline-full.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- config/forge/resolve_job.yaml
- config/forge/workflows/tasks.yaml
- tests/test_scheduling.py
- tests/test_shutdown.py
- tests/test_lifecycle.py
Collaborator
|
/test ? |
Collaborator
|
/test deploy-fournos-wip |
3 similar comments
Collaborator
|
/test deploy-fournos-wip |
Collaborator
|
/test deploy-fournos-wip |
Collaborator
|
/test deploy-fournos-wip |
|
🔴 Test of 'fournos_deploy --project-source' failed after 00 hours 00 minutes 00 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Collaborator
|
/test deploy-fournos-wip |
|
🔴 Test of 'fournos_deploy --project-source' failed after 00 hours 00 minutes 00 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
…t vault validation
Collaborator
|
/test deploy-fournos-wip |
|
🟢 Test of 'fournos_deploy --project-source' succeeded after 00 hours 13 minutes 07 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
Signed-off-by: avasilev <avasilev@redhat.com>
Signed-off-by: avasilev <avasilev@redhat.com>
avasilevskii
changed the title
Collaborator
|
tested in openshift-psap/forge#51 (comment) thanks @avasilevskii , |
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.
forgeresolver and use Tekton Pipeline Annotations to supply the forge imageCloses #62.
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores
Tests