HIVE-2862: Build containers without python deps#2865
HIVE-2862: Build containers without python deps#2865suhanime wants to merge 2 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@suhanime: This pull request references HIVE-2862 which is a valid jira issue. 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. |
|
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:
📝 WalkthroughWalkthroughReplaced Python-based utilities with Bash equivalents: removed Changes
Sequence Diagram(s)sequenceDiagram
participant Makefile
participant VersionScript as hack/version2.sh
participant GitRepo as Git
participant YQ as yq
participant TemplateScript as hack/app-sre/generate-saas-template.sh
participant Build as Docker/Build
Makefile->>VersionScript: compute SOURCE_GIT_TAG
VersionScript->>GitRepo: resolve COMMIT, BRANCH, COMMIT_COUNT, short SHA
VersionScript-->>Makefile: return version string
Makefile->>TemplateScript: invoke (YQ=$(YQ)) to build saas template
TemplateScript->>YQ: count & extract objects from saas_object_file
TemplateScript->>TemplateScript: merge objects into template stub, write out_file
Makefile->>Build: pass SOURCE_GIT_TAG into build
Build-->>Makefile: produce artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
📝 Coding Plan
Comment |
|
@suhanime: This pull request references HIVE-2862 which is a valid jira issue. 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. |
![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
🧹 Nitpick comments (1)
hack/version2.sh (1)
29-44: Consider adding level prefix to log messages.The
logfunction accepts alevelparameter but doesn't include it in the output. This makes it harder to distinguish debug, info, warning, and fatal messages when reading logs.♻️ Optional: Add level prefix to log output
log() { local level="$1" shift local message="$*" if [[ "$MODE" == "standalone" ]]; then - echo "$message" >&2 + echo "[$level] $message" >&2 else - echo "$message" + echo "[$level] $message" fi if [[ "$level" == "fatal" ]]; then exit 1 fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/version2.sh` around lines 29 - 44, The log function currently accepts a level parameter but prints only the message; update the log() function to prefix printed output with the level (e.g., "[level] message") so callers can distinguish debug/info/warn/fatal; preserve existing MODE logic (when MODE=="standalone" write to stderr, otherwise stdout) and keep the exit behavior when level == "fatal". Ensure you modify the log() implementation (references: function name log, parameter level, variable MODE) to build and echo the prefixed string rather than echoing just "$message".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/version2.sh`:
- Around line 291-294: The version_string() function currently echoes
"v$(semver)" which duplicates the 'v' because the Makefile's SOURCE_GIT_TAG :=
v$(shell hack/version2.sh) also prepends a 'v'; fix by removing the leading 'v'
from version_string() so it echoes "$(semver)" (leave semver() behavior
unchanged), or alternatively remove the extra 'v' in the Makefile assignment to
avoid producing "vv..."—preferably update version_string() to return the raw
semver value to keep tag formatting centralized.
In `@Makefile`:
- Around line 83-85: SOURCE_GIT_TAG is being set to v$(shell hack/version2.sh)
which causes a double "v" when version_string() already emits a leading "v"; fix
by ensuring only one source of the "v" prefix: either remove the hardcoded "v"
in SOURCE_GIT_TAG or (preferred) change version_string() in hack/version2.sh to
return the version without a leading "v" so SOURCE_GIT_TAG remains v<version>.
Update the version_string() implementation accordingly and keep SOURCE_GIT_TAG
as-is to preserve the existing Makefile pattern.
---
Nitpick comments:
In `@hack/version2.sh`:
- Around line 29-44: The log function currently accepts a level parameter but
prints only the message; update the log() function to prefix printed output with
the level (e.g., "[level] message") so callers can distinguish
debug/info/warn/fatal; preserve existing MODE logic (when MODE=="standalone"
write to stderr, otherwise stdout) and keep the exit behavior when level ==
"fatal". Ensure you modify the log() implementation (references: function name
log, parameter level, variable MODE) to build and echo the prefixed string
rather than echoing just "$message".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1df7309-73af-414c-9abb-ca8841a1c1a4
📒 Files selected for processing (5)
DockerfileMakefilehack/ubi-build-deps.shhack/version2.pyhack/version2.sh
💤 Files with no reviewable changes (2)
- Dockerfile
- hack/version2.py
openshift-ci
bot
added
the
approved
|
@suhanime: This pull request references HIVE-2862 which is a valid jira issue. 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. |
This change allows building the container images without the python dependencies that are getting harder to get by. As a part of this change, version2.py is faithfully converted to a shell script of the same name. Co-authored-by: Antoni Segura Puimedon <antoni@redhat.com> Co-authored-by: Mark Old <mold@redhat.com> Assisted-by: Claude Sonnet 4.5
|
@suhanime: This pull request references HIVE-2862 which is a valid jira issue. 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. |
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 `@hack/app-sre/generate-saas-template.sh`:
- Around line 23-33: Move the help-flag detection to run before the strict
arg-count validation and match only exact help flags (e.g., "-h" or "--help")
instead of the broad pattern "*-h*"; in practice, run the existing for-loop that
calls usage and exits prior to the `if [ $# -ne 3 ]; then ...` block and update
the condition to test exact equality (for example `[[ "$arg" == "-h" || "$arg"
== "--help" ]]`) so legitimate file paths containing "-h" are not treated as
help requests.
In `@hack/version2.sh`:
- Around line 35-39: The log() implementation currently writes to stdout when
MODE="library", which contaminates command-substitution outputs; update the
branch that handles MODE="library" so it writes logs to stderr instead (use the
same stderr redirection used for the standalone branch), ensuring all calls to
log() (and any helper that echoes messages) do not emit to stdout and thus won't
corrupt function return values like branch/commit resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0264e957-fb96-4366-8fda-a6b1e18ceb73
📒 Files selected for processing (8)
DockerfileMakefilehack/app-sre/generate-saas-template.pyhack/app-sre/generate-saas-template.shhack/app-sre/saas-template-stub.yamlhack/ubi-build-deps.shhack/version2.pyhack/version2.sh
💤 Files with no reviewable changes (3)
- Dockerfile
- hack/version2.py
- hack/app-sre/generate-saas-template.py
✅ Files skipped from review due to trivial changes (1)
- hack/app-sre/saas-template-stub.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- hack/ubi-build-deps.sh
| if [ $# -ne 3 ]; then | ||
| usage | ||
| exit 1 | ||
| fi | ||
|
|
||
| for arg in "$@"; do | ||
| if [[ "$arg" == *"-h"* ]]; then | ||
| usage | ||
| exit 1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Help flag detection is too broad and can reject valid file paths.
The check on Line 29 (*-h*) matches any argument containing -h, so legitimate paths can trigger help mode. Please match only exact help flags and handle help before strict arg-count validation.
💡 Suggested fix
-# Check for help flag or incorrect number of arguments
-if [ $# -ne 3 ]; then
- usage
- exit 1
-fi
-
-for arg in "$@"; do
- if [[ "$arg" == *"-h"* ]]; then
- usage
- exit 1
- fi
-done
+# Check for help flag first
+if [[ "${1:-}" == "-h" || "${1:-}" == "--help" ]]; then
+ usage
+ exit 0
+fi
+
+# Validate argument count
+if [ $# -ne 3 ]; then
+ usage
+ exit 1
+fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/app-sre/generate-saas-template.sh` around lines 23 - 33, Move the
help-flag detection to run before the strict arg-count validation and match only
exact help flags (e.g., "-h" or "--help") instead of the broad pattern "*-h*";
in practice, run the existing for-loop that calls usage and exits prior to the
`if [ $# -ne 3 ]; then ...` block and update the condition to test exact
equality (for example `[[ "$arg" == "-h" || "$arg" == "--help" ]]`) so
legitimate file paths containing "-h" are not treated as help requests.
There was a problem hiding this comment.
The rabbit isn't wrong here. Right now this will work... but only because none of the paths we're actually passing in have -h as a substring. If some day one of the files is renamed to something like saas-template-for-hive it'll blow up and it'll be pretty hard to figure out why.
That said, I see where this is a faithful port of the same mistake from the python script. If you'd like to argue that this PR should be limited to an exact port and we'll fix things like this later, I can buy it.
There was a problem hiding this comment.
@2uasimojo, agreed on the risk — a path like saas-template-for-hive or anything with -h as a substring would silently trigger help mode and exit 1, which would be genuinely confusing to debug.
Given that the fix is already outlined and amounts to just a few lines (move the check, tighten the pattern, and flip the help-exit to exit 0), fixing it in this PR seems like the lower-friction path rather than carrying forward a known footgun. But ultimately that's the author's call!
| if [[ "$MODE" == "standalone" ]]; then | ||
| echo "$message" >&2 | ||
| else | ||
| echo "$message" | ||
| fi |
There was a problem hiding this comment.
Library-mode logging can corrupt function return values.
When MODE="library", log() writes to stdout, but multiple helpers also return data via stdout and are used in command substitutions. This can contaminate parsed values (e.g., branch commit resolution) and cause incorrect behavior when sourced as a library.
💡 Suggested fix
- if [[ "$MODE" == "standalone" ]]; then
- echo "$message" >&2
- else
- echo "$message"
- fi
+ echo "$message" >&2📝 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.
| if [[ "$MODE" == "standalone" ]]; then | |
| echo "$message" >&2 | |
| else | |
| echo "$message" | |
| fi | |
| echo "$message" >&2 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/version2.sh` around lines 35 - 39, The log() implementation currently
writes to stdout when MODE="library", which contaminates command-substitution
outputs; update the branch that handles MODE="library" so it writes logs to
stderr instead (use the same stderr redirection used for the standalone branch),
ensuring all calls to log() (and any helper that echoes messages) do not emit to
stdout and thus won't corrupt function return values like branch/commit
resolution.
suhanime
changed the title
|
@suhanime: This pull request references HIVE-2862 which is a valid jira issue. 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
generate-saas-template.py is used by make verify target, switch it to a shell script. Switching yaml to yq should generate functionally identical outputs, with yq preserving the original order of fields as well as quotation marks. There was a minor bug in the python script where sys.exit was missed if the files provided as arguments were not present, which is fixed in the shell script. Assisted-by: Claude Sonnet 4.5
suhanime
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Note for the reviewer: Please hide whitespaces when reviewing. yq vs yaml has ended up in the following changes from what I can tell
- comments from the script are preserved now
- entries are more deterministic because yq preserves order
- added indentation at a few place that - specifically for description field
- string entries are interpreted better and are more readable now
There was a problem hiding this comment.
Cool. Here's how I reviewed this:
Changes to description fields are not significantly functional, so after I glanced through several pages to validate that the changes weren't crazy-looking, I removed all description fields from both copies (yq-v4.47.1 -i 'del(.. | .description?)' $file)
The remaining delta comprised:
- New version has comments at the top. You mentioned this... but they're not comments from the script -- they're the comments that originated in the input template file. Adding
| .. comments=""eliminates, uhh, the first comment block but not the second?? But that ends up making the result make more sense. - String values are double- rather than single-quoted where quotes are required; and not quoted at all where not required. This is fine.
- The
parameters:section stays at the top in the new version, vs being shoved to the bottom in the old. This is goodness 👍
So nit/followup, add that comment scrubber.
2uasimojo
left a comment
2uasimojo
left a comment
There was a problem hiding this comment.
/lgtm
A few small issues noted inline, but I'm happy to see them addressed in a fup.
Note that this still isn't producing the version string we want:
efried@efried-thinkpadp16vgen1:~/go/src/github.com/openshift/hive$ podman run quay.io/redhat-user-workloads/crt-redhat-acm-tenant/hive-operator/hive:on-pr-aebbfa253c77130b2b5305e632df14a6929b64ef
time="2026-03-20T23:02:12.13Z" level=info msg="Enabling pprof" pprof_host_port="localhost:6060"
time="2026-03-20T23:02:12.13Z" level=info msg="Version: openshift/hive " <==== L@@K
time="2026-03-20T23:02:12.13Z" level=fatal msg="invalid configuration: no configuration has been provided, try setting KUBERNETES_MASTER environment variable"
...which may or may not resolve when this is built on-push. If not, we'll still have something to tackle via HIVE-3045.
| if [ $# -ne 3 ]; then | ||
| usage | ||
| exit 1 | ||
| fi | ||
|
|
||
| for arg in "$@"; do | ||
| if [[ "$arg" == *"-h"* ]]; then | ||
| usage | ||
| exit 1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
The rabbit isn't wrong here. Right now this will work... but only because none of the paths we're actually passing in have -h as a substring. If some day one of the files is renamed to something like saas-template-for-hive it'll blow up and it'll be pretty hard to figure out why.
That said, I see where this is a faithful port of the same mistake from the python script. If you'd like to argue that this PR should be limited to an exact port and we'll fix things like this later, I can buy it.
There was a problem hiding this comment.
So yeah, I'm not going to bother reviewing this thoroughly. I did test it against version2.py for three scenarios:
- master
- mce-2.8
- an uncommitted branch
...and it produces the same output, so I'm gonna call it good.
Very soon, I want to simplify it so it only operates on master and uncommitted, which should make it a much smaller thing.
There was a problem hiding this comment.
Cool. Here's how I reviewed this:
Changes to description fields are not significantly functional, so after I glanced through several pages to validate that the changes weren't crazy-looking, I removed all description fields from both copies (yq-v4.47.1 -i 'del(.. | .description?)' $file)
The remaining delta comprised:
- New version has comments at the top. You mentioned this... but they're not comments from the script -- they're the comments that originated in the input template file. Adding
| .. comments=""eliminates, uhh, the first comment block but not the second?? But that ends up making the result make more sense. - String values are double- rather than single-quoted where quotes are required; and not quoted at all where not required. This is fine.
- The
parameters:section stays at the top in the new version, vs being shoved to the bottom in the old. This is goodness 👍
So nit/followup, add that comment scrubber.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo, suhanime The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@suhanime: 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. |
4 participants
This change allows building the container images without the python dependencies that are getting harder to get by.
As a part of this change, version2.py is faithfully converted to a shell script of the same name.
Co-authored-by: Antoni Segura Puimedon antoni@redhat.com
Co-authored-by: Mark Old mold@redhat.com
Assisted-by: Claude Sonnet 4.5
Summary by CodeRabbit