ART-21084: derive OKD enablement from group.yml okd.enabled#3096
ART-21084: derive OKD enablement from group.yml okd.enabled#3096lgarciaaco wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@lgarciaaco: This pull request references ART-21084 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
WalkthroughAdds ChangesOKD enablement flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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.
Inline comments:
In `@pyartcd/pyartcd/pipelines/okd_images_health.py`:
- Around line 48-55: In okd_images_health.py, the run() flow in the OKD images
health pipeline should stop when no enabled versions are resolved. After the
versions are set via the _versions_param branch or
util.get_okd_enabled_versions(), add an early skip/return path in run() for the
empty self.versions case so the notification block is not reached and false “all
healthy” messages are not emitted.
In `@pyartcd/pyartcd/pipelines/okd_scan.py`:
- Around line 91-94: Move the _check_params() validation ahead of the
is_okd_version_enabled() lookup in okd_scan.py so the trust-boundary check runs
before any build-data access; keep the early build-data gating in the same
method that currently performs the version check, and ensure self.data_path and
self.data_gitref are only dereferenced after parameters have been validated.
🪄 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: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 297d91e9-6147-4ca9-9308-2c34a6cf3137
📒 Files selected for processing (9)
ocp-build-data-validator/tests/test_schema/test_group_schema.pyocp-build-data-validator/validator/json_schemas/assembly_group_config.schema.jsonpyartcd/pyartcd/constants.pypyartcd/pyartcd/pipelines/okd_images_health.pypyartcd/pyartcd/pipelines/okd_scan.pypyartcd/pyartcd/util.pypyartcd/tests/pipelines/test_okd_images_health.pypyartcd/tests/pipelines/test_okd_scan.pypyartcd/tests/test_util.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pyartcd/pyartcd/pipelines/okd_images_health.py (1)
79-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn early when no OKD versions are resolved.
If
_resolve_versions()returns[](no--versionsor every requested version is disabled),run()still falls through to the notification path and can emit a false “all healthy” message even though nothing was scanned.Suggested fix
async def run(self): self.versions = await self._resolve_versions() + if not self.versions: + self.runtime.logger.info("No enabled OKD versions to scan; skipping health check") + return await asyncio.gather(*(self.get_report(v) for v in self.versions)) await asyncio.gather(*(self.get_rebase_failures(v) for v in self.versions)) self.runtime.logger.info('Found %s concerns', len(self.report))🤖 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 `@pyartcd/pyartcd/pipelines/okd_images_health.py` around lines 79 - 90, The run() method in okd_images_health.Pipeline should stop before scanning or notification when _resolve_versions() returns no versions. Add an early return right after self._resolve_versions() if self.versions is empty, so the later asyncio.gather calls and notify_release_channel/notify_okd_channel paths in run() are skipped when nothing was resolved.pyartcd/pyartcd/pipelines/okd_scan.py (1)
91-106: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMove
_check_params()ahead of the build-data probe.Line 92 still touches the caller-supplied build-data source before Line 106 enforces the non-dry-run restrictions, so a rejected custom
data_path/data_gitrefis consulted before the trust-boundary check runs.Suggested fix
async def run(self): """ Main pipeline execution. """ # If we get here, lock could be acquired self.skipped = False + self._check_params() # Early exit if version not enabled for OKD in build-data if not await util.is_okd_version_enabled(self.doozer_base_command): self.logger.info( 'Version %s is not enabled for OKD (set okd.enabled: true in group.yml on openshift-%s). Skipping scan.', @@ - self._check_params() - # Scan for changes await self._scan_sources()As per path instructions, "Validate at trust boundaries with allow-lists, not deny-lists".
🤖 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 `@pyartcd/pyartcd/pipelines/okd_scan.py` around lines 91 - 106, Move the parameter validation in `OKDScanPipeline.run` so `_check_params()` runs before any build-data access such as `util.is_okd_version_enabled()` or logging derived from `data_path`/`data_gitref`. This ensures the trust-boundary check happens first and rejected caller-supplied inputs are not consulted before validation; keep the early exit and scan logging after `_check_params()` once the inputs are accepted.Source: Path instructions
🤖 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.
Duplicate comments:
In `@pyartcd/pyartcd/pipelines/okd_images_health.py`:
- Around line 79-90: The run() method in okd_images_health.Pipeline should stop
before scanning or notification when _resolve_versions() returns no versions.
Add an early return right after self._resolve_versions() if self.versions is
empty, so the later asyncio.gather calls and
notify_release_channel/notify_okd_channel paths in run() are skipped when
nothing was resolved.
In `@pyartcd/pyartcd/pipelines/okd_scan.py`:
- Around line 91-106: Move the parameter validation in `OKDScanPipeline.run` so
`_check_params()` runs before any build-data access such as
`util.is_okd_version_enabled()` or logging derived from
`data_path`/`data_gitref`. This ensures the trust-boundary check happens first
and rejected caller-supplied inputs are not consulted before validation; keep
the early exit and scan logging after `_check_params()` once the inputs are
accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f70c313-cd55-46bb-a3ce-9ff4413ebebd
📒 Files selected for processing (6)
pyartcd/pyartcd/constants.pypyartcd/pyartcd/pipelines/okd_images_health.pypyartcd/pyartcd/pipelines/okd_scan.pypyartcd/pyartcd/util.pypyartcd/tests/pipelines/test_okd_images_health.pypyartcd/tests/test_util.py
💤 Files with no reviewable changes (1)
- pyartcd/pyartcd/constants.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pyartcd/pyartcd/util.py`:
- Around line 142-143: The `enabled` check in `util.py` is too permissive
because `bool(enabled)` treats non-boolean values like strings as truthy; update
the `enabled` handling in the version-check logic to only return true for the
literal boolean `True`, and treat `Missing`, `False`, and any other non-boolean
value as disabled. Keep the fix localized to the existing
`runtime.get_group_config().enabled` flow so the trust-boundary behavior in this
helper is strict and explicit.
🪄 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: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69a8cb77-9d7b-4613-b8ff-a69e89454d9c
📒 Files selected for processing (4)
pyartcd/pyartcd/pipelines/okd_images_health.pypyartcd/pyartcd/pipelines/okd_scan.pypyartcd/pyartcd/util.pypyartcd/tests/test_util.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyartcd/pyartcd/pipelines/okd_images_health.py
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Ran okd-scan dry-runs on Jenkins against 4.23 skip (build #763) 4.21 happy path (build #765) Passed the gate ( Earlier failures (#762, #764) were bad distgit keys, not enablement: Local pyartcd tests still green (12 okd-related). Marking PRs ready for review. |
5cc7840 to
a0486e3
Compare
locriandev
left a comment
locriandev
left a comment
There was a problem hiding this comment.
IIUC, with this change schedule_okd_scan will spawn a build/okd-scan job for each OCP version. Then, okd-scan will check for OKD enable status and skip if needed. This will produce a bunch of [skipped] Jenkins runs. I would rather move the enabled check to the scheduled pipeline, so that we'd trigger build/okd-scan only when actually needed
|
[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 |
a86f236 to
08bfbe5
Compare
Replace OKD_ENABLED_VERSIONS with build-data lookups via doozer --variant=okd and config:read-group enabled. okd-scan and okd-images-health skip disabled versions; schedule-okd-scan probes build-data and gates before triggering build/okd-scan Jenkins jobs so disabled versions do not produce skipped downstream runs. Also add okd.enabled to ocp-build-data validator schema, discover enabled versions when --version/--versions are omitted, and fix ocp4-scan-konflux unit tests for the skip_rpms constructor arg after rebasing onto main. rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
08bfbe5 to
ce16ca5
Compare
lgarciaaco
changed the title
|
@lgarciaaco: 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. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
3 participants
Summary
Move OKD version enablement from the hardcoded
OKD_ENABLED_VERSIONSlist tookd.enabledin ocp-build-datagroup.yml. Pipelines read merged group config through doozer--variant=okd.What changed
OKD_ENABLED_VERSIONSfrompyartcd/constants.pyis_okd_version_enabled(),get_okd_enabled_versions(), andokd_doozer_base_command()inpyartcd/util.pyokd.enabledis false or missing (still needed for manual Jenkins runs)--versionsthrough the enablement gate; probeACTIVE_OCP_VERSIONSwhen omitted; exit early when nothing is enabled--versionis omitted; filter explicit--versionargs; check enablement inrun_for()beforestart_okd_scan_konfluxso disabled versions do not spawn skippedbuild/okd-scanjobsokd.enabledunderokd:inassembly_group_config.schema.jsonskip_rpms=Falseintest_ocp4_scan_konflux.pyafter rebase onto main (Add --skip-rpms parameter to ocp4-scan-konflux pipeline #3115)Dependencies
okd.enabled: trueon 4.21, 4.22, 5.0; 4.23 stays disabled)okdVersionsloop, callsschedule-okd-scanwith no--version)Validation
pytest pyartcd/tests/pipelines/test_okd_scan.py pyartcd/tests/pipelines/test_okd_images_health.py pyartcd/tests/pipelines/test_schedule_okd_scan.py pyartcd/tests/test_util.pyJira: ART-21084