Draft
Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The apollo-ci image tag
stackrox-test-0.5.1-5-ga9b384f054/scanner-test-0.5.1-5-ga9b384f054is repeated across many workflows; consider centralizing this in a single variable (e.g., viaenvor a reusable workflow) to keep future updates consistent. - The
central-entrypoint.shroot-handling block currently has aTODO (DO NOT MERGE)and relies onsu-exec, which is noted as missing; please either restore a working user-switching mechanism for UBI9 or remove/adjust this conditional before merging. - In
scanner/image/scanner/Dockerfile, thesave-dir-contentscall was narrowed to/etc/pki/ca-trust/source; verify that corresponding restore logic and comments are updated to reflect this narrower scope so it’s clear what is actually persisted.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The apollo-ci image tag `stackrox-test-0.5.1-5-ga9b384f054`/`scanner-test-0.5.1-5-ga9b384f054` is repeated across many workflows; consider centralizing this in a single variable (e.g., via `env` or a reusable workflow) to keep future updates consistent.
- The `central-entrypoint.sh` root-handling block currently has a `TODO (DO NOT MERGE)` and relies on `su-exec`, which is noted as missing; please either restore a working user-switching mechanism for UBI9 or remove/adjust this conditional before merging.
- In `scanner/image/scanner/Dockerfile`, the `save-dir-contents` call was narrowed to `/etc/pki/ca-trust/source`; verify that corresponding restore logic and comments are updated to reflect this narrower scope so it’s clear what is actually persisted.
## Individual Comments
### Comment 1
<location> `image/rhel/static-bin/central-entrypoint.sh:21-26` </location>
<code_context>
echo >&2 "Warning: failed to change permissions of one or more directories. Startup may fail."
fi
+ # TODO(DO NOT MERGE): exec: su-exec: not found
+ # Is this entire conditional still relevant? If so, perhaps we can make it
+ # better.
+ # Doesn't seem to work in UBI8 either.
+ # Also, the conditional itself isn't POSIX-compliant (should be `[ "$(id -u)" = "0" ]`)
exec su-exec 4000:4000 "$0" "$@"
fi
</code_context>
<issue_to_address>
**issue (bug_risk):** Committing a `DO NOT MERGE` TODO and relying on `su-exec` (likely absent in UBI9) is risky.
This still unconditionally calls `exec su-exec ...`, so on UBI9/UBI8 the container will likely fail to start because `su-exec` isn’t present. If UID dropping is still needed, consider switching to something available in the base image (e.g. `runuser`, `gosu`, or `setpriv`), or remove this branch if everything now runs as the target UID. Also, the `DO NOT MERGE` TODO should be resolved or removed before committing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Images are ready for the commit at f16b33c. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18364 +/- ##
==========================================
+ Coverage 49.65% 49.68% +0.03%
==========================================
Files 2698 2700 +2
Lines 203132 203297 +165
==========================================
+ Hits 100860 101014 +154
- Misses 94748 94757 +9
- Partials 7524 7526 +2
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:
|
Contributor
Author
|
/test ocp-4-20-qa-e2e-tests |
709ad18 to
0354dd6
Compare
efdc34e to
84236ee
Compare
e2cc9c0 to
c9e89ad
Compare
c9e89ad to
515f592
Compare
Contributor
Author
|
/test gke-nongroovy-e2e-tests |
Contributor
Author
|
/test gke-qa-e2e-tests |
Contributor
Author
|
/test gke-nongroovy-e2e-tests |
a05e9b2 to
448ded1
Compare
Contributor
Author
|
/test gke-qa-e2e-tests |
|
@github-actions[bot]: The The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
Update ImageDigestMirrorSet, COMPONENT_MAPPINGS, and operator-bundle pipeline to reference rhel9 image repositories instead of rhel8. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update all hardcoded rhel8 image names to rhel9 in: - pkg/images/defaults/flavor.go (RHACS release flavor) - deploy/k8s/sensor-deploy/chart/internal/defaults/50-images.yaml Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Go builder: rhel_8_golang_1.25 -> rhel_9_golang_1.25 - Runtime: ubi8/ubi-minimal -> ubi9/ubi-minimal - PostgreSQL: rhel8/postgresql-15 -> rhel9/postgresql-15 - Labels: rhacs-*-rhel8 -> rhacs-*-rhel9 - Stripped stale sha256 digests (will be repinned by Mintmaker) Affected images: main, roxctl, central-db, operator, operator-bundle, scanner-v4, scanner-v4-db Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update ARG defaults and FROM references from ubi8/rhel8 to ubi9/rhel9 in development/CI Dockerfiles. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update helm test expectations, scanner mappers test, and release verification script to reference rhel9 image names. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
411fc15 to
ec08506
Compare
Change pg_rhel_major from 8 to 9 in download scripts so PostgreSQL RPMs are fetched from the RHEL9 PGDG repository. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The brew.registry.redhat.io requires pinned digests for image pulls. Use the known-good digest for rhel_9_golang_1.25 from the scanner PR. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update image_reference_regex helper and interactive flavors test to expect rhel9 image names in generated deployment manifests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update flavor-interactive.expect.tcl to match rhel9 image names in the RHACS special-case patterns for central-db, main, scanner, scanner-db, scanner-v4, and scanner-v4-db. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In UBI9, update-ca-trust requires the -o flag to specify the output directory when running as a non-root user. This matches the workaround applied in the scanner repo (PR #2562). See: https://bugzilla.redhat.com/show_bug.cgi?id=2241240 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- .tekton/*-build.yaml: CPE labels el8 -> el9 - operator/Dockerfile: ubi9-micro -> ubi9-minimal (matches Konflux) - rpms.rhel.repo: RHEL 8 repos -> RHEL 9 Note: rpms.lock.yaml needs regeneration (not included in this commit). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/konflux-retest main-on-push |
UBI9-minimal's microdnf does not support modularity. PostgreSQL 15 is available directly in UBI9 repos without needing module enable. This was causing the main-on-push Konflux build to fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/konflux-retest main-on-push |
6e594af to
cc5ed9f
Compare
Contributor
Author
|
/konflux-retest operator-bundle-on-push |
On UBI9-minimal, python3.12 is installed but python3 is not automatically symlinked. Use alternatives to create the symlink, matching Moritz's fix in mc/ubi9. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cc5ed9f to
4b6558f
Compare
deploy/k8s/sensor-deploy/chart/internal/defaults/50-images.yaml is gitignored and generated at deploy time from flavor.go defaults. Should not be committed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Konflux build environment has entitlement certificates and needs module enable to resolve postgresql 15 from the RHEL 9 appstream. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/konflux-retest main-on-push |
The lock file must reference RHEL 9 repos for Konflux prefetch to work with UBI9 base images. Previous commits failed to persist this change. Sourced from Moritz's mc/ubi9 branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
/konflux-retest operator-bundle-on-push |
2 similar comments
Contributor
|
/konflux-retest operator-bundle-on-push |
Contributor
Author
|
/konflux-retest operator-bundle-on-push |
Contributor
Author
|
/konflux-retest main-on-push |
Contributor
Author
|
/konflux-retest operator-bundle-on-push |
Contributor
Author
|
/konflux-retest create-custom-snapshot |
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.
Description
base image migration from UBI/rhel 8 to 9.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!