Repository navigation
[fournos-launcher] Make it work with FOURNOS v0.3.1 - #21
Merged
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 |
|
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:
📝 WalkthroughWalkthroughCI entrypoint now branches on FOURNOS_CI: Fournos-specific env/config files are processed and converted into variable_overrides saved under ARTIFACT_DIR/000__ci_metadata; the OpenShift CI path is isolated. All CI env vars are written to env.sh (best-effort). PR metadata is fetched via requests into pull_request*.json artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Entrypoint
participant FS as Artifact Dir (000__ci_metadata)
participant PRapi as Pull Request API
participant Fournos as Fournos Launcher/CLI
participant Kube as Kubernetes (oc)
CI->>CI: parse_and_save_pr_arguments() checks FOURNOS_CI
alt FOURNOS_CI=true
CI->>FS: process_fournos_environment() (read/move forge_config.env)
CI->>CI: transform_fournos_config_to_variable_overrides()
CI->>FS: write variable_overrides.yaml and move originals
else ocpci
CI->>CI: parse_and_save_pr_arguments_ocpci()
CI->>FS: write variable_overrides (ocpci path)
end
CI->>FS: system_prechecks() write env.sh (best-effort)
CI->>PRapi: download_pr_information() -> requests.get(...)
PRapi-->>FS: write pull_request.json / pull_request-comments.json
Fournos->>Kube: submit() creates job (renders job.yaml with spec.forge.args)
Fournos->>Kube: cleanup_job() -> oc delete fournosjob {job} -n {namespace} --ignore-not-found
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
…te to the lastest CRD
Contributor
Author
|
/test jump-ci llm_d cks |
Contributor
Author
|
/test fournos llm_d cks |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
projects/core/ci_entrypoint/prepare_ci.py (1)
399-403: Defensive check: Consider adding shell identifier validation toenv.shgeneration.The code exports environment variables without validating that keys are valid shell identifiers (pattern
^[A-Za-z_][A-Za-z0-9_]*$). A test confirmed that no invalid identifiers currently exist in the CI environment (invalid_shell_identifier_count=0), so the script works correctly today. However, adding explicit filtering or validation before writing would make this more robust against unexpected environment states.🤖 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 399 - 403, The env export loop in prepare_ci.py writes all os.environ items to env.sh without validating that keys are valid shell identifiers; update the loop that iterates over os.environ (the for key, value in sorted(os.environ.items()) block) to first validate each key against the shell identifier regex ^[A-Za-z_][A-Za-z0-9_]*$ and only write exports for matching keys, skipping (and optionally logging) any invalid keys; keep the existing escaping of values (escaped_value = value.replace("'", "'\\''")) and write only validated entries to the file.
🤖 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 393-403: The current loop that writes all environment variables
(the for key, value in sorted(os.environ.items()) block that computes
escaped_value and f.write export lines) will persist secrets; change this to
either write only an allowlist (e.g., a module-level ALLOWLIST_ENV_VARS) or
redact sensitive values by checking key against a blacklist/regex (e.g., keys
matching TOKEN|SECRET|KEY|PASSWORD|AWS|GCP) and replacing the value with
"<REDACTED>" before escaping and writing; additionally, after creating env_file
set restrictive permissions (use os.chmod(env_file, 0o600)) so the artifact is
not world-readable.
In `@projects/fournos_launcher/orchestration/cli.py`:
- Around line 76-79: Validate the incoming namespace string before persisting
it: check the `namespace` variable matches a safe Kubernetes/DNS label regex
(e.g. lowercase letters and digits with optional internal hyphens, start/end
with alphanumeric, max length 63) and reject or raise/log an error for anything
that doesn't match; only call `config.project.set_config("fournos.namespace",
namespace)` after the validation passes and keep the existing
`logging.info(f"Using namespace {namespace}")` placement after successful
validation to avoid storing or using unsafe input in subsequent `oc` command
interpolation.
In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py`:
- Around line 213-215: The cleanup call using shell.run(f'oc delete fournosjob
{ctx.final_job_name} -n {args.namespace} --ignore-not-found') must be made
best-effort so it cannot mask the primary submit/wait failure; wrap that
shell.run invocation (the oc delete using ctx.final_job_name and args.namespace)
in a try/except that catches any exception, does not re-raise, and instead logs
a warning (including the exception message and the job name/namespace) so
cleanup failures are non-fatal when the function is decorated with `@always`.
- Line 217: Replace the stale return string "No manifest file to clean up" with
a message that reflects the actual resource being deleted (e.g., "No FournosJob
resource to clean up" or "No FournosJob found to clean up"); search for that
exact string in main.py (the return value in the cleanup function) and update it
so logs/returns correctly reference the FournosJob resource instead of a
manifest file.
---
Nitpick comments:
In `@projects/core/ci_entrypoint/prepare_ci.py`:
- Around line 399-403: The env export loop in prepare_ci.py writes all
os.environ items to env.sh without validating that keys are valid shell
identifiers; update the loop that iterates over os.environ (the for key, value
in sorted(os.environ.items()) block) to first validate each key against the
shell identifier regex ^[A-Za-z_][A-Za-z0-9_]*$ and only write exports for
matching keys, skipping (and optionally logging) any invalid keys; keep the
existing escaping of values (escaped_value = value.replace("'", "'\\''")) and
write only validated entries to the file.
🪄 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: 38310976-368c-4c39-af48-93ca87dfd4d0
📒 Files selected for processing (4)
projects/core/ci_entrypoint/prepare_ci.pyprojects/fournos_launcher/orchestration/cli.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 26 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
Contributor
Author
|
/test fournos llm_d cks |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 26 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d cks |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 26 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d cks |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 26 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d cks |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 26 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
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 `@projects/core/library/config.py`:
- Around line 91-93: The loop overwrites existing ci_job entries by
unconditionally setting self.config["ci_job"]["name"/"project"/"args"] = None;
change it to only initialize missing keys (e.g., use setdefault or check if the
key is absent) so existing values are preserved; modify the code that touches
self.config["ci_job"] in this module (the section currently iterating for name
in "name", "project", "args") to only assign None when the key is not 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: fc61b47f-052c-42c9-a998-ec97d5ac0a9e
📒 Files selected for processing (1)
projects/core/library/config.py
Contributor
Author
|
/test fournos llm_d cks |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 26 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
projects/core/ci_entrypoint/prepare_ci.py (1)
206-209: MakeFOURNOS_CIdetection tolerant to common truthy values.Line 206 currently requires exact
'true', which can silently route to the wrong parser for values likeTrue/1/yes.Suggested adjustment
- if os.environ.get('FOURNOS_CI') == 'true': + fournos_ci = os.environ.get('FOURNOS_CI', '').strip().lower() in ('1', 'true', 'yes', 'on') + if fournos_ci: return parse_and_save_pr_arguments_fournos() else: return parse_and_save_pr_arguments_ocpci()🤖 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 206 - 209, The current check os.environ.get('FOURNOS_CI') == 'true' is too strict; replace it by normalizing the env var and checking against common truthy values (e.g., val = os.environ.get('FOURNOS_CI', '').strip().lower(); if val in {'1','true','yes','on'}: return parse_and_save_pr_arguments_fournos() else: return parse_and_save_pr_arguments_ocpci()) so parse_and_save_pr_arguments_fournos and parse_and_save_pr_arguments_ocpci are chosen correctly for inputs like "True", "1", "yes", or "on".
🤖 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 212-242: The function
transform_fournos_config_to_variable_overrides should validate the incoming
forge_config schema and types before using keys like 'project', 'args', and
'configOverrides'; specifically ensure forge_config is a dict, that 'project'
and 'args' are of expected types (or missing), and that config_overrides =
forge_config.get('configOverrides', {}) is a dict — if not, raise a clear
ValueError (or coerce only when safe) explaining the invalid FOURNOS config
instead of allowing a subsequent TypeError; update the function to perform these
checks at the top and use the validated values when setting 'project.name',
'project.args', and merging configOverrides.
---
Nitpick comments:
In `@projects/core/ci_entrypoint/prepare_ci.py`:
- Around line 206-209: The current check os.environ.get('FOURNOS_CI') == 'true'
is too strict; replace it by normalizing the env var and checking against common
truthy values (e.g., val = os.environ.get('FOURNOS_CI', '').strip().lower(); if
val in {'1','true','yes','on'}: return parse_and_save_pr_arguments_fournos()
else: return parse_and_save_pr_arguments_ocpci()) so
parse_and_save_pr_arguments_fournos and parse_and_save_pr_arguments_ocpci are
chosen correctly for inputs like "True", "1", "yes", or "on".
🪄 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: f1edcf6e-29fa-4c82-80af-734e1c8a007f
📒 Files selected for processing (1)
projects/core/ci_entrypoint/prepare_ci.py
Comment on lines
+212
to
+242
| def transform_fournos_config_to_variable_overrides(forge_config: dict) -> dict: | ||
| """ | ||
| Transform FOURNOS forge_config format to variable_overrides format. | ||
|
|
||
| Args: | ||
| forge_config: Dictionary containing FOURNOS config with keys: | ||
| - project: project name | ||
| - args: list of arguments | ||
| - configOverrides: dict of config overrides | ||
|
|
||
| Returns: | ||
| Dictionary in variable_overrides format: | ||
| - project -> project.name | ||
| - args -> project.args | ||
| - configOverrides entries are flattened directly | ||
| """ | ||
| variable_overrides = {} | ||
|
|
||
| # Transform project -> project.name | ||
| if 'project' in forge_config: | ||
| variable_overrides['project.name'] = forge_config['project'] | ||
|
|
||
| # Transform args -> project.args | ||
| if 'args' in forge_config: | ||
| variable_overrides['project.args'] = forge_config['args'] | ||
|
|
||
| # Add all configOverrides entries directly (flatten them) | ||
| config_overrides = forge_config.get('configOverrides', {}) | ||
| variable_overrides.update(config_overrides) | ||
|
|
||
| return variable_overrides |
There was a problem hiding this comment.
Validate FOURNOS config schema before transforming/writing.
Line 269 can return None/non-dict, and Lines 239-240 assume configOverrides is a dict. This can fail with opaque errors instead of a clear config validation failure.
Proposed hardening
def transform_fournos_config_to_variable_overrides(forge_config: dict) -> dict:
@@
- variable_overrides = {}
+ if not isinstance(forge_config, dict):
+ raise ValueError("forge_config.yaml must contain a top-level mapping")
+
+ variable_overrides = {}
@@
- if 'args' in forge_config:
- variable_overrides['project.args'] = forge_config['args']
+ if 'args' in forge_config:
+ if not isinstance(forge_config['args'], list):
+ raise ValueError("forge_config.yaml: 'args' must be a list")
+ variable_overrides['project.args'] = forge_config['args']
@@
- config_overrides = forge_config.get('configOverrides', {})
+ config_overrides = forge_config.get('configOverrides', {})
+ if config_overrides is None:
+ config_overrides = {}
+ if not isinstance(config_overrides, dict):
+ raise ValueError("forge_config.yaml: 'configOverrides' must be a mapping")
variable_overrides.update(config_overrides)Also applies to: 268-270, 283-287
🤖 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 212 - 242, The
function transform_fournos_config_to_variable_overrides should validate the
incoming forge_config schema and types before using keys like 'project', 'args',
and 'configOverrides'; specifically ensure forge_config is a dict, that
'project' and 'args' are of expected types (or missing), and that
config_overrides = forge_config.get('configOverrides', {}) is a dict — if not,
raise a clear ValueError (or coerce only when safe) explaining the invalid
FOURNOS config instead of allowing a subsequent TypeError; update the function
to perform these checks at the top and use the validated values when setting
'project.name', 'project.args', and merging configOverrides.
Contributor
Author
|
/test fournos llm_d cks |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 26 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 27 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d cks |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 27 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d cks |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 55 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d cks |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 06 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos llm_d cks |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 16 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
|
@kpouget: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Contributor
Author
|
/test fournos llm_d cks |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 00 minutes 27 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
kpouget
changed the title
Contributor
Author
|
test passed, merging this 👍🏻 |
This was referenced Apr 24, 2026
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
Improvements