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. |
github-actions
bot
added
area/operator
area/ci
konflux-build
|
/konflux-retest central-db-on-push |
1 similar comment
|
/konflux-retest central-db-on-push |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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.
| # 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 | ||
|
|
||
| # This can be adjusted if deploying into non-standard namespaces. For example, as in |
There was a problem hiding this comment.
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.
| # 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 |
There was a problem hiding this comment.
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.
📝 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 | ✅ 3✅ Passed checks (3 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 |
![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 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
| 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 |
There was a problem hiding this comment.
Potential PyYAML version drift between local and Konflux builds.
The Makefile-based local build installs PyYAML==6.0 via pip from requirements.txt, while this Dockerfile installs python3.12-pyyaml via microdnf without version pinning. This could lead to different PyYAML versions being used in different build environments, potentially causing subtle differences in YAML output formatting.
Additionally, .python-version specifies Python 3.9 but this Dockerfile uses Python 3.12. While likely not breaking, it's worth noting the inconsistency.
Consider verifying that the system package version matches the pinned version, or document that version differences are acceptable for this use case.
🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operator/konflux.bundle.Dockerfile` around lines 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.
| # 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 |
There was a problem hiding this comment.
Comment is inconsistent with Dockerfile.
The comment states "The Konflux build uses Python 3.9" but operator/konflux.bundle.Dockerfile actually installs python3.12-pyyaml and configures Python 3.12 via alternatives.
Suggested fix
-# 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 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.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 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 | |
| # 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 ?= python3 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operator/Makefile` around lines 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.
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 |
porridge
changed the title
porridge
changed the title
1 participant
This reverts commit 15ab6e7 from PR #18947
Description
Does not work on konflux:
Slack thread.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!