fix(ci): Revert "ROX-27677: Eliminate Python dependency...#19860
fix(ci): Revert "ROX-27677: Eliminate Python dependency...#19860
Conversation
|
Skipping CI for Draft Pull Request. |
|
/konflux-retest central-db-on-push |
1 similar comment
|
/konflux-retest central-db-on-push |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The Python version references are inconsistent: the Makefile comment mentions Python 3.9 while the Konflux Dockerfile installs Python 3.12 and the .python-version file is referenced as the source of truth; please align the comments and tooling so they clearly reflect and enforce a single required Python version.
- In ACTIVATE_PYTHON you explicitly pin pip/setuptools to versions noted as incompatible with Python 3.12 in the preceding comment; if Konflux is using Python 3.12, consider either relaxing these pins or updating them to versions known to work with 3.12 to avoid build-time import errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Python version references are inconsistent: the Makefile comment mentions Python 3.9 while the Konflux Dockerfile installs Python 3.12 and the .python-version file is referenced as the source of truth; please align the comments and tooling so they clearly reflect and enforce a single required Python version.
- In ACTIVATE_PYTHON you explicitly pin pip/setuptools to versions noted as incompatible with Python 3.12 in the preceding comment; if Konflux is using Python 3.12, consider either relaxing these pins or updating them to versions known to work with 3.12 to avoid build-time import errors.
## Individual Comments
### Comment 1
<location path="operator/Makefile" line_range="15-19" />
<code_context>
-# Python binary to use for bundle helpers. Can be overridden to use a specific version (e.g., python3.12).
-# The Konflux build uses Python 3.12, so using python3.12 locally ensures compatibility.
+# Python binary to use for bundle helpers. Can be overridden to use a specific version (e.g., python3.9).
+# The Konflux build uses Python 3.9, so using python3.9 locally ensures compatibility.
PYTHON ?= python3
</code_context>
<issue_to_address>
**issue (bug_risk):** Pinned pip/setuptools versions are incompatible with Python 3.12 but the Konflux stack still uses 3.12, which is likely to break venv setup.
Konflux’s Dockerfile still installs and defaults to python3.12, while `ACTIVATE_PYTHON` now pins `pip==21.3.1` and `setuptools==59.6.0`, which you’ve noted don’t support Python 3.12 (`pkgutil.ImpImporter` removal). In that environment, `pip install` is likely to fail in `bundle-post-process` or `test-bundle-helpers`. Please either: (1) align the base image Python version with `.python-version` (e.g., 3.9), or (2) choose pip/setuptools versions compatible with Python 3.12, so the configuration is consistent and doesn’t break at runtime.
</issue_to_address>
### Comment 2
<location path="operator/konflux.bundle.Dockerfile" line_range="3-5" />
<code_context>
- alternatives --install /usr/bin/python3 python3 /usr/bin/python3.12 1
+FROM registry.access.redhat.com/ubi9/ubi-minimal:latest@sha256:69f5c9886ecb19b23e88275a5cd904c47dd982dfa370fbbd0c356d7b1047ef68 AS builder
+
+# This installs both PyYAML and Python.
+# The alternatives command ensures that the python3 command points to the installed Python 3.12, which is required by the bundle generation scripts.
+RUN microdnf -y install python3.12-pyyaml && alternatives --install /usr/bin/python3 python3 /usr/bin/python3.12 1
COPY . /stackrox
</code_context>
<issue_to_address>
**issue (bug_risk):** Konflux image forces Python 3.12 while the Makefile tooling now assumes an older Python for pinned pip/setuptools versions.
This container forces `/usr/bin/python3` to be 3.12, but the Makefile’s `ACTIVATE_PYTHON` target pins `pip==21.3.1` and `setuptools==59.6.0`, which depend on `pkgutil.ImpImporter` removed in Python 3.12. That mismatch can break `bundle-post-process` and `test-bundle-helpers`. Please either align the image with the Python version in `bundle_helpers/.python-version` (and point `python3` there) or update the pinned pip/setuptools versions to ones compatible with 3.12.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
📝 WalkthroughWalkthroughThis diff removes the Go-based bundle helper implementation entirely, consolidating around Python-only bundle manifest post-processing. Changes include deleting Go source and test files, wrapper/comparison scripts, supporting documentation, and adjusting build infrastructure (Makefile, Dockerfile) to directly invoke Python scripts and use a Python-capable base image instead of a Go builder. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operator/konflux.bundle.Dockerfile`:
- Around line 1-5: The Dockerfile installs python3.12-pyyaml via microdnf (RUN
microdnf -y install python3.12-pyyaml && alternatives --install ...) which can
produce a different PyYAML version than the pinned PyYAML==6.0 used in local
builds; update the builder stage to ensure PyYAML is version-pinned (either
install system package with the exact 6.0 RPM if available, or remove the system
PyYAML and instead install Python and pip then pip install -r requirements.txt /
pip install PyYAML==6.0) and make the Python major.minor consistent with
.python-version (either use Python 3.9 in the Dockerfile or document/standardize
on 3.12) so builds use the same YAML library and Python runtime across
environments.
In `@operator/Makefile`:
- Around line 15-17: Update the Makefile comment and optionally the default to
match the Dockerfile: change the comment that currently says "The Konflux build
uses Python 3.9" to state the project uses Python 3.12 (to match
konflux.bundle.Dockerfile), and consider setting the PYTHON variable default to
python3.12 (replace PYTHON ?= python3) so local builds mirror the container
setup; update the comment near the PYTHON variable and the PYTHON assignment
accordingly.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 726ac1c1-16b8-4490-ba94-d0088fb28ced
📒 Files selected for processing (24)
.github/workflows/test-bundle-helpers.yaml.tekton/operator-bundle-build.yamloperator/Makefileoperator/bundle_helpers/README.mdoperator/bundle_helpers/cmd/fix_descriptors.gooperator/bundle_helpers/cmd/output.gooperator/bundle_helpers/cmd/patch_csv.gooperator/bundle_helpers/compare-implementations.shoperator/bundle_helpers/dispatch.shoperator/bundle_helpers/main.gooperator/bundle_helpers/pkg/csv/patcher.gooperator/bundle_helpers/pkg/csv/patcher_test.gooperator/bundle_helpers/pkg/csv/version.gooperator/bundle_helpers/pkg/csv/version_test.gooperator/bundle_helpers/pkg/descriptor/sorter.gooperator/bundle_helpers/pkg/descriptor/sorter_test.gooperator/bundle_helpers/pkg/rewrite/rewriter.gooperator/bundle_helpers/pkg/rewrite/rewriter_test.gooperator/bundle_helpers/pkg/values/values.gooperator/bundle_helpers/pkg/values/values_test.gooperator/bundle_helpers/prepare-bundle-manifests.shoperator/bundle_helpers/requirements.txtoperator/bundle_helpers/yaml-normalizer.pyoperator/konflux.bundle.Dockerfile
💤 Files with no reviewable changes (19)
- operator/bundle_helpers/yaml-normalizer.py
- operator/bundle_helpers/pkg/rewrite/rewriter.go
- operator/bundle_helpers/pkg/rewrite/rewriter_test.go
- operator/bundle_helpers/pkg/values/values_test.go
- operator/bundle_helpers/pkg/csv/patcher_test.go
- operator/bundle_helpers/README.md
- operator/bundle_helpers/main.go
- operator/bundle_helpers/dispatch.sh
- operator/bundle_helpers/compare-implementations.sh
- operator/bundle_helpers/cmd/fix_descriptors.go
- operator/bundle_helpers/cmd/output.go
- operator/bundle_helpers/pkg/descriptor/sorter_test.go
- operator/bundle_helpers/pkg/csv/patcher.go
- operator/bundle_helpers/pkg/csv/version_test.go
- operator/bundle_helpers/pkg/csv/version.go
- operator/bundle_helpers/cmd/patch_csv.go
- operator/bundle_helpers/pkg/descriptor/sorter.go
- .github/workflows/test-bundle-helpers.yaml
- operator/bundle_helpers/pkg/values/values.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19860 +/- ##
==========================================
- Coverage 49.60% 49.55% -0.05%
==========================================
Files 2763 2758 -5
Lines 208339 207855 -484
==========================================
- Hits 103342 103006 -336
+ Misses 97332 97222 -110
+ Partials 7665 7627 -38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 Build Images ReadyImages are ready for commit 156f64e. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-570-g156f64e5e4 |
|
/retest operator-bundle-on-push |
This comment was marked as outdated.
This comment was marked as outdated.
|
/konflux-retest operator-bundle-on-push |
2 similar comments
|
/konflux-retest operator-bundle-on-push |
|
/konflux-retest operator-bundle-on-push |
|
😢 |
This reverts commit 15ab6e7 from PR #18947
Description
Does not work on konflux:
build log output
Slack thread - including results of failed quick fix attempts.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
CI is enough.