Hive-3130: bundle-gen rewrite + port to shell#2916
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@suhanime: This pull request references Hive-3130 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces the Python bundle generator with a Bash script implementing CLI parsing, image validation/build, semver and previous-version discovery, bundle and CSV generation (with operatorhub extras), automated PR creation; removes Python helpers and updates small helper scripts and CSV template comments. ChangesBundle Generation Migration
Supporting Script Improvements
Sequence Diagram(s)sequenceDiagram
participant Local as hack/bundle-gen.sh
participant Quay as Quay API
participant HiveRepo as Hive repo
participant Community as community-operators-prod
participant Fork as Fork repo
participant GH as gh CLI
Local->>Quay: validate image tag (validate_image)
Local->>HiveRepo: clone target Hive repo & checkout commit
Local->>Community: scan bundles to get previous version (get_previous_version)
Local->>Fork: push update-hive-<version> branch with bundle (open_pr)
Local->>GH: create PR via gh pr create (open_pr)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@hack/bundle-gen.sh`:
- Around line 400-404: The script uses git -C "$repo_path" checkout -b
"$branch_name" which fails if the branch already exists; change this to git -C
"$repo_path" checkout -B "$branch_name" so the branch is force-created/updated
(allowing re-runs that correspond with the later --force push), referencing the
existing variables branch_name, repo_path, and semver_ver when locating the line
to modify.
- Around line 298-311: The loop currently iterates over every regular file in
crds_dir which allows non-YAML files to be processed by yq; change the find
invocation used to feed crd_file to only match YAML files (for example replace
find "$crds_dir" -maxdepth 1 -type f with find "$crds_dir" -maxdepth 1 -type f
\( -iname '*.yaml' -o -iname '*.yml' \) | sort) or add a guard before processing
each crd_file that skips files not ending in .yaml/.yml; update the use sites of
crd_file/kind/version_name/etc. accordingly so only YAML CRDs are copied and
passed to yq.
- Around line 162-166: The curl call that assigns the local variable resp can
hang; modify the command in the resp assignment inside the conditional (the
block using "if ! resp=$(curl -sf "$url"); then") to include a timeout flag
(e.g. --max-time <seconds> or --connect-timeout <seconds>) so the command fails
fast on unresponsive endpoints (for example use --max-time 30); keep the -s and
-f flags and preserve the existing error/exit handling.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6a868823-77d5-47a8-81cb-4907b313c111
📒 Files selected for processing (7)
config/templates/hive-csv-template.yamlhack/app-sre/generate-saas-template.shhack/bundle-gen.pyhack/bundle-gen.shhack/github.pyhack/requirements.txthack/version2.sh
💤 Files with no reviewable changes (3)
- hack/requirements.txt
- hack/github.py
- hack/bundle-gen.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hack/bundle-gen.sh (2)
302-310: 💤 Low valueConsider handling missing CRD descriptions gracefully.
If a CRD lacks
.spec.versions[0].schema.openAPIV3Schema.description,yqreturns the literal string"null", which would appear verbatim in the CSV's owned CRDs list.♻️ Optional: default to empty string for missing description
- description=$(yq '.spec.versions[0].schema.openAPIV3Schema.description' "$crd_file") + description=$(yq '.spec.versions[0].schema.openAPIV3Schema.description // ""' "$crd_file")🤖 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 `@hack/bundle-gen.sh` around lines 302 - 310, The script currently assigns description=$(yq '.spec.versions[0].schema.openAPIV3Schema.description' "$crd_file") which can yield the literal "null"; update the logic around the description variable (and the subsequent yq invocation that uses DESCRIPTION) to normalize missing descriptions to an empty string. Concretely, after computing description (or by adjusting the yq query), ensure if description is "null" or empty it is replaced with "" before exporting DESCRIPTION and running the final yq -i command that appends the CSV entry (refer to the variables description, DESCRIPTION and the yq append invocation). This will prevent the literal "null" from appearing in the owned CRDs list.
118-123: 💤 Low valueConsider validating that value-options receive an actual value.
If a user accidentally passes
--github-user --verbose(missing the username),GITHUB_USERgets set to--verbose, causing a confusing failure later inopen_pr. Similarly, passing--github-userat the end of the command line would hit an unbound variable error with an unclear message.♻️ Optional: add a helper to validate required arguments
+require_arg() { + if [[ -z "${2:-}" || "$2" == --* ]]; then + echo "Error: $1 requires a value" >&2 + exit 1 + fi +} + parse_args() { while [[ $# -gt 0 ]]; do case "$1" in --verbose) VERBOSE=true ;; --dry-run) DRY_RUN=true ;; --hold) HOLD=true ;; --skip-release-config) SKIP_RELEASE_CONFIG=true ;; --skip-image-validation) SKIP_IMAGE_VALIDATION=true ;; - --github-user) GITHUB_USER="$2"; shift ;; - --hive-repo) HIVE_REPO="$2"; shift ;; - --image-repo) IMAGE_REPO="$2"; shift ;; - --image-tag-override) IMAGE_TAG_OVERRIDE="$2"; shift ;; - --commit) COMMIT_ISH="$2"; shift ;; - --dummy-bundle) DUMMY_BUNDLE="$2"; shift ;; + --github-user) require_arg "$1" "${2:-}"; GITHUB_USER="$2"; shift ;; + --hive-repo) require_arg "$1" "${2:-}"; HIVE_REPO="$2"; shift ;; + --image-repo) require_arg "$1" "${2:-}"; IMAGE_REPO="$2"; shift ;; + --image-tag-override) require_arg "$1" "${2:-}"; IMAGE_TAG_OVERRIDE="$2"; shift ;; + --commit) require_arg "$1" "${2:-}"; COMMIT_ISH="$2"; shift ;; + --dummy-bundle) require_arg "$1" "${2:-}"; DUMMY_BUNDLE="$2"; shift ;; -h|--help) usage; exit 0 ;;🤖 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 `@hack/bundle-gen.sh` around lines 118 - 123, When parsing flags in hack/bundle-gen.sh ensure value-options (e.g., --github-user, --hive-repo, --image-repo, --image-tag-override, --commit, --dummy-bundle) verify the next token is a real value (not another flag or empty) before assigning to GITHUB_USER, HIVE_REPO, IMAGE_REPO, IMAGE_TAG_OVERRIDE, COMMIT_ISH, or DUMMY_BUNDLE; add a small helper like require_value_or_exit that checks $2 exists and does not start with "-" and call it from each case branch (or inline the check) to print a clear error and exit, so missing values don't propagate into open_pr or cause unbound-variable failures.
🤖 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.
Nitpick comments:
In `@hack/bundle-gen.sh`:
- Around line 302-310: The script currently assigns description=$(yq
'.spec.versions[0].schema.openAPIV3Schema.description' "$crd_file") which can
yield the literal "null"; update the logic around the description variable (and
the subsequent yq invocation that uses DESCRIPTION) to normalize missing
descriptions to an empty string. Concretely, after computing description (or by
adjusting the yq query), ensure if description is "null" or empty it is replaced
with "" before exporting DESCRIPTION and running the final yq -i command that
appends the CSV entry (refer to the variables description, DESCRIPTION and the
yq append invocation). This will prevent the literal "null" from appearing in
the owned CRDs list.
- Around line 118-123: When parsing flags in hack/bundle-gen.sh ensure
value-options (e.g., --github-user, --hive-repo, --image-repo,
--image-tag-override, --commit, --dummy-bundle) verify the next token is a real
value (not another flag or empty) before assigning to GITHUB_USER, HIVE_REPO,
IMAGE_REPO, IMAGE_TAG_OVERRIDE, COMMIT_ISH, or DUMMY_BUNDLE; add a small helper
like require_value_or_exit that checks $2 exists and does not start with "-" and
call it from each case branch (or inline the check) to print a clear error and
exit, so missing values don't propagate into open_pr or cause unbound-variable
failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0a66c34-d5de-472a-ae7e-e332d87373db
📒 Files selected for processing (7)
config/templates/hive-csv-template.yamlhack/app-sre/generate-saas-template.shhack/bundle-gen.pyhack/bundle-gen.shhack/github.pyhack/requirements.txthack/version2.sh
💤 Files with no reviewable changes (3)
- hack/requirements.txt
- hack/bundle-gen.py
- hack/github.py
✅ Files skipped from review due to trivial changes (1)
- config/templates/hive-csv-template.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/version2.sh
- hack/app-sre/generate-saas-template.sh
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/bundle-gen.sh (1)
474-474: 💤 Low valueDefensive: use
--before positional arguments in git checkout.While
COMMITis controlled byversion_initand typically a SHA, adding--prevents accidental flag interpretation if the value ever starts with-.Proposed fix
- git checkout "$COMMIT" + git checkout -- "$COMMIT"🤖 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 `@hack/bundle-gen.sh` at line 474, The git checkout invocation using COMMIT is vulnerable if COMMIT begins with a dash; update the checkout call in the script so the positional argument is separated from flags by adding the explicit separator: invoke git checkout with the `--` separator before "$COMMIT" (i.e., change the git checkout "$COMMIT" usage to use `git checkout -- "$COMMIT"`), ensuring the COMMIT variable is still quoted to preserve whitespace.
🤖 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.
Nitpick comments:
In `@hack/bundle-gen.sh`:
- Line 474: The git checkout invocation using COMMIT is vulnerable if COMMIT
begins with a dash; update the checkout call in the script so the positional
argument is separated from flags by adding the explicit separator: invoke git
checkout with the `--` separator before "$COMMIT" (i.e., change the git checkout
"$COMMIT" usage to use `git checkout -- "$COMMIT"`), ensuring the COMMIT
variable is still quoted to preserve whitespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 65a4c0ce-2499-4d24-b2a1-c424a31af06a
📒 Files selected for processing (7)
config/templates/hive-csv-template.yamlhack/app-sre/generate-saas-template.shhack/bundle-gen.pyhack/bundle-gen.shhack/github.pyhack/requirements.txthack/version2.sh
💤 Files with no reviewable changes (3)
- hack/requirements.txt
- hack/github.py
- hack/bundle-gen.py
✅ Files skipped from review due to trivial changes (1)
- config/templates/hive-csv-template.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/app-sre/generate-saas-template.sh
- hack/version2.sh
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Rewrites the OperatorHub bundle generation and publishing script from Python to bash, eliminating all Python dependencies (GitPython, PyYAML, semver, requests) in favour of standard tools already present in the environment: git, yq (v4), jq, gh CLI. Key changes: - bundle-gen.sh replaces bundle-gen.py with identical CLI surface and behaviour. Usage: same flags, same $GITHUB_TOKEN env var. - github.py and requirements.txt are removed; the new script uses `gh pr create` instead of the custom GitHub REST client. - version2.sh is sourced directly instead of the now-deleted version2.py. - Adds early dependency check (check_deps) that fails immediately with a clear error if yq, jq, git, or gh are not on PATH — preventing cascading silent failures mid-run. - Fixes a latent Python bug: --dummy-bundle without --skip-release-config caused a TypeError (str + None) crash. The shell version separates generate_bundle from add_operatorhub_extras so this cannot occur. - Fixes createdAt annotation: Python used local time mislabeled as UTC (datetime.now() with a literal Z suffix); shell uses date -u correctly. - Fixes version2.sh log() routing: in library mode log messages wrote to stdout, corrupting command substitutions like branch_commit=$(find_branch). All log levels now always write to stderr. - Temp-dir cleanup is guaranteed via EXIT trap rather than only on clean exit. - --verbose now works (set -x); was dead code in the Python version. - Dead code removed: generate_package() (never called) and SUBPROCESS_REDIRECT (set but never passed to any subprocess call). Assisted-by: Claude Sonnet 4.6
…ate.sh Follow-up to CodeRabbit's suggestion on openshift#2865. The help detection loop matched any argument containing the substring "-h" (e.g. a file path like /some/hive-template.yaml would trigger help mode and exit). Fix by: - Checking for -h / --help before the argument-count validation, so help works even when the wrong number of args is supplied. - Using exact string matching instead of glob *-h*. - Exiting with status 0 on help (was erroneously exiting 1). Assisted-by: Claude Sonnet 4.6
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 `@hack/version2.sh`:
- Around line 202-207: The mapfile -t pipelines for the arrays here, ancestors
and descendants are turning empty arrays into a single empty-string entry
because printf '%s\n' emits a blank line; update the three mapfile invocations
(the ones that read into here, ancestors, descendants) to filter out blank lines
before mapfile (e.g., pipe the printf output through a command that deletes
empty lines such as sed '/^$/d') or alternatively wrap each mapfile call in a
short guard that only runs when the source array has elements; ensure the change
preserves the existing set -u-safe empty-array expansion idiom and only affects
those three mapfile pipelines.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f5ede1ca-3b50-4205-9597-38dcfeadc2de
📒 Files selected for processing (7)
config/templates/hive-csv-template.yamlhack/app-sre/generate-saas-template.shhack/bundle-gen.pyhack/bundle-gen.shhack/github.pyhack/requirements.txthack/version2.sh
💤 Files with no reviewable changes (3)
- hack/requirements.txt
- hack/github.py
- hack/bundle-gen.py
🚧 Files skipped from review as they are similar to previous changes (3)
- config/templates/hive-csv-template.yaml
- hack/app-sre/generate-saas-template.sh
- hack/bundle-gen.sh
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@hack/bundle-gen.sh`:
- Around line 500-503: The script copies the new bundle into an existing
directory which causes nested output on reruns; before calling cp -r for the
generated bundle (in the block using generate_bundle, BUNDLE_DIR, ver_semver and
dest), ensure the previous output is removed or replaced by deleting the
existing destination directory (e.g., check for existence of dest and rm -rf it)
or use a copy command that replaces the target rather than nesting; update the
logic around dest to remove/replace the existing
hive-operator-bundle-v${ver_semver} before copying.
- Around line 449-458: The check_deps function currently omits curl so
validate_image() can later fail; add "curl" to the list of commands checked in
check_deps (the for cmd in ... loop) and include it in the error/help output so
the script fails fast when curl is missing; locate check_deps and update both
the for-loop's command list and the printed dependency lines to mention curl.
- Around line 509-515: The script currently only rejects equal versions
(ver_semver == prev_version) which allows generating bundles with a lower
version; update the guard in hack/bundle-gen.sh to perform a proper semver
comparison and fail unless ver_semver is strictly greater than prev_version
(i.e., treat ver_semver <= prev_version as an error). Locate the block around
ver_semver/prev_version and the calls to generate_bundle and
add_operatorhub_extras and replace the equality check with a semantic-version
comparison (for example using sort -V, version sort, or a small semver compare
function) so the script exits with an error if ver_semver is less-than or
equal-to prev_version.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c17393d9-b1a3-4d66-a3f0-92456c845faf
📒 Files selected for processing (7)
config/templates/hive-csv-template.yamlhack/app-sre/generate-saas-template.shhack/bundle-gen.pyhack/bundle-gen.shhack/github.pyhack/requirements.txthack/version2.sh
💤 Files with no reviewable changes (3)
- hack/requirements.txt
- hack/github.py
- hack/bundle-gen.py
✅ Files skipped from review due to trivial changes (1)
- config/templates/hive-csv-template.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/version2.sh
- hack/app-sre/generate-saas-template.sh
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 `@hack/bundle-gen.sh`:
- Around line 483-487: The call to version_init currently passes an empty second
arg when COMMIT_ISH is unset, causing version_init to treat it as HEAD; change
the call so the second argument is COMMIT_ISH if provided, otherwise explicitly
pass the requested branch value (the same value passed as the first
arg/DUMMY_BUNDLE) so version_init resolves the branch tip instead of using
whatever the repo has checked out; update the invocation that uses DUMMY_BUNDLE
and COMMIT_ISH accordingly so the subsequent checkout logic operates on the
intended branch tip.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9d6b3f1a-a766-47d1-8bd3-42a94a64c1a1
📒 Files selected for processing (2)
hack/bundle-gen.shhack/version2.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/version2.sh
| # version2.sh sets globals: COMMIT, BRANCH_NAME, PREFIX, COMMIT_COUNT | ||
| # In OperatorHub mode the branch is always master; --dummy-bundle overrides | ||
| # this with an explicit branch (e.g. mce-2.1). Passing the branch explicitly | ||
| # avoids ambiguous auto-detection when the repo has many branches. | ||
| version_init "${DUMMY_BUNDLE:-master}" "${COMMIT_ISH:-}" |
There was a problem hiding this comment.
Make the no---commit path resolve the requested branch tip.
version_init() treats an empty second argument as HEAD (hack/version2.sh:244-255), so this still builds from whatever the clone has checked out. That breaks the documented defaults for both --dummy-bundle <branch> and --hive-repo pointing at a local repo whose HEAD is not master: Line 494 checks out the wrong commit, and the generated semver follows it.
Proposed fix
- version_init "${DUMMY_BUNDLE:-master}" "${COMMIT_ISH:-}"
+ local target_branch="${DUMMY_BUNDLE:-master}"
+ version_init "$target_branch" "${COMMIT_ISH:-$target_branch}"🤖 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 `@hack/bundle-gen.sh` around lines 483 - 487, The call to version_init
currently passes an empty second arg when COMMIT_ISH is unset, causing
version_init to treat it as HEAD; change the call so the second argument is
COMMIT_ISH if provided, otherwise explicitly pass the requested branch value
(the same value passed as the first arg/DUMMY_BUNDLE) so version_init resolves
the branch tip instead of using whatever the repo has checked out; update the
invocation that uses DUMMY_BUNDLE and COMMIT_ISH accordingly so the subsequent
checkout logic operates on the intended branch tip.
There was a problem hiding this comment.
This would break the local clone usage, which is usually how the users execute bundle-gen currently. While the lack of commit if dummy-bundle option is used (with a non-master branch) might result in error, it will still come up with a useful failure message.
There was a problem hiding this comment.
@suhanime, that's a fair point — for local clones, HEAD is exactly what the user wants to build from, and overriding it with the branch name as a fallback would break that common workflow. I'll defer to your judgment here.
✏️ Learnings added
Learnt from: suhanime
Repo: openshift/hive PR: 2916
File: hack/bundle-gen.sh:483-487
Timestamp: 2026-06-05T06:42:02.632Z
Learning: In `hack/bundle-gen.sh` (openshift/hive), the call `version_init "${DUMMY_BUNDLE:-master}" "${COMMIT_ISH:-}"` intentionally passes an empty second argument when `--commit` is not specified. This is by design: the primary use case is pointing `--hive-repo` at a local directory, where `HEAD` is the intended commit to build from. Passing the branch name as a fallback for the second arg would break local clone usage. Do not suggest changing this to `version_init "$target_branch" "${COMMIT_ISH:-$target_branch}"`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: suhanime
Repo: openshift/hive PR: 2901
File: apis/vendor/github.com/openshift/api/config/v1/types_authentication.go:290-307
Timestamp: 2026-05-14T04:23:24.966Z
Learning: In the openshift/hive repository, vendor files (both at the repository root vendor/ directory and under apis/vendor/) must never be modified directly. Issues in vendored code should be addressed by bumping the dependency version in go.mod (or apis/go.mod) to pull in a version that contains the upstream fix. Do not raise review comments suggesting edits to vendored files.
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
| # Deduplicate by logical branch name: the same branch appearing under multiple | ||
| # remotes (e.g. refs/heads/master and refs/remotes/origin/master both resolve | ||
| # to "master") should be counted once. | ||
| mapfile -t here < <(printf '%s\n' "${here[@]+"${here[@]}"}" | sort -u | grep -v '^$') | ||
| mapfile -t ancestors < <(printf '%s\n' "${ancestors[@]+"${ancestors[@]}"}" | sort -u | grep -v '^$') | ||
| mapfile -t descendants < <(printf '%s\n' "${descendants[@]+"${descendants[@]}"}" | sort -u | grep -v '^$') |
There was a problem hiding this comment.
I can't parse this. If all we're trying to do is deduplicate, can we just make here, ancestors, and descendants associative arrays?
@coderabbitai Can you break down this goofy syntax for me? I get the sort and grep. I get that mapfile is consuming a "file" with newlines and putting the contents into an array such that each line (minus the newline character) is one array entry. The bit in the middle is a mystery though. Talk me through from the first < to the last ".
There was a problem hiding this comment.
@2uasimojo Sure! Here's the breakdown of "${here[@]+"${here[@]}"}":
The core: ${parameter+word}
This is bash's use-alternative parameter expansion. The rule:
- If
parameteris unset or null, substitute nothing. - If
parameteris set and non-null, substituteword.
Applying it to arrays: ${here[@]+...}
When the [@] subscript is used with the + operator on an empty array, bash treats it as "unset/null", so the whole thing expands to nothing. When the array has elements, it expands to word.
The reason this idiom exists at all is set -u (nounref), which is active here via set -euo pipefail. Without protection, ${here[@]} on an empty array causes a fatal "unbound variable" error in some bash versions.
The full expression: "${here[@]+"${here[@]}"}"
Breaking it apart:
| Part | Meaning |
|---|---|
Outer "..." |
Outer double-quotes wrapping the whole expansion |
${here[@]+ |
Check: does here have any elements? |
"${here[@]}" |
The word: expand all array elements as separate arguments |
}" |
Close the ${...} and the outer quote |
Result:
hereis empty → expands to nothing →printf '%s\n'gets no args → no outputhereis non-empty → expands to"${here[@]}"→printf '%s\n' "elem1" "elem2" ...→ one element per line
Then sort -u deduplicates the lines, and grep -v '^$' strips any blank lines, and mapfile -t here reads the result back into the array.
Your associative array suggestion
Yes, that would be cleaner and more idiomatic. Instead of local -a here=(), you'd use:
declare -A here=()
declare -A ancestors=()
declare -A descendants=()Then replace here+=("$refname") with here["$refname"]=1, and similarly for the others. Deduplication is free since a key can only appear once. Later, ${#here[@]} still gives the count, and "${!here[@]}" gives the keys (branch names). The awkward mapfile/printf/sort block goes away entirely.
The only wrinkle: the downstream code uses ${here[0]}, ${here[1]}, etc. (indexed access). Associative arrays don't guarantee order and don't support numeric indexing, so you'd need to convert to an indexed array first when reading out, e.g. mapfile -t here_arr < <(printf '%s\n' "${!here[@]}") before doing ${here_arr[0]}. Whether that's a net improvement in readability is debatable.
(¬_¬") 🐇
There was a problem hiding this comment.
The "downstream code" is only consuming the contents if there's exactly one entry; so something like
if [[ ${#here[@]} -eq 1 ]]; then
log "debug" "Found unique branch ${here[0]} at commit $(shortsha)"
echo "${!here[@]}" # <== prints "all keys", but there's only one.
return 0
fi...would work fine, no?
There was a problem hiding this comment.
@2uasimojo Yes, exactly right — ${!here[@]} when count is 1 gives the single key (the branch name), so the echo works perfectly.
The ${here[0]} in the debug log line also needs updating though, and ${descendants[*]} would print all 1s (the values) instead of branch names. Here's what the full downstream change would look like:
if [[ ${`#here`[@]} -eq 1 ]]; then
log "debug" "Found unique branch ${!here[@]} at commit $(shortsha)"
echo "${!here[@]}"
return 0
fi
if [[ ${`#here`[@]} -gt 1 ]]; then
log "fatal" "Found ${`#here`[@]} branches at commit $(shortsha)! Please specify a branch name explicitly."
fi
if [[ ${`#descendants`[@]} -eq 1 ]]; then
log "debug" "Found branch ${!descendants[@]} descended from commit $(shortsha)"
echo "${!descendants[@]}"
return 0
fi
log "debug" "descendants: ${!descendants[*]}"
if [[ ${`#ancestors`[@]} -eq 1 ]]; then
log "debug" "Found branch ${!ancestors[@]} which is an ancestor of commit $(shortsha)"
echo "${!ancestors[@]}"
return 0
fiAnd the array declarations + population:
local -A here=()
local -A ancestors=()
local -A descendants=()
# ...
here["$refname"]=1
# or
ancestors["$refname"]=1
# or
descendants["$refname"]=1That eliminates the mapfile/printf/sort block entirely. Net improvement in readability, I'd say.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Bug: when bundle-gen was invoked with --commit pointing to a commit that
differs from the cloned repo's HEAD, version_init in version2.sh skipped
branch auto-detection entirely, leaving BRANCH_NAME empty. This caused
the version prefix to fall back to UNKNOWN_BRANCH_PREFIX ("0.0") instead
of the correct "1.2" for master, producing invalid bundle versions that
violate semver ordering against existing bundles.
This was introduced in f1bc37a when version2.py was ported to
version2.sh. The port missed the fallback to _branch_from_commit() that
the Python version called when the active branch HEAD did not match the
requested commit.
Fix in version2.sh:
- When the active branch HEAD does not match COMMIT, fall through to
branch_from_commit to discover the correct branch.
- Deduplicate the here/ancestors/descendants arrays in branch_from_commit
by logical branch name before uniqueness checks, so that the same branch
appearing under multiple remotes (e.g. refs/heads/master and
refs/remotes/origin/master) is counted once rather than causing a
spurious "Found N branches" fatal error.
Fix in bundle-gen.sh:
- Explicitly pass "master" as the branch argument to version_init in
OperatorHub mode (with --dummy-bundle overriding this when targeting a
specific non-master branch such as mce-2.1). The version2.sh
auto-detection is not reliable when the local repo has many branches
that are all descendants of the requested commit.
Versioning guards added to OperatorHub mode to catch distinct failure
cases before they can reach the community-operator repos:
- Prefix check: verifies that the computed version prefix ($PREFIX)
matches MASTER_BRANCH_PREFIX before cloning community-operators-prod.
Fails fast if branch detection fell back to the unknown prefix.
- Existence check: rejects the bundle if the computed version is identical
to the current upstream head with a clear "already exists" error.
- Ordering check: rejects the bundle if the computed version is lower than
the current upstream head, which can happen if a valid-looking prefix
was produced but the commit count has already been superseded.
The three checks are kept separate because they indicate distinct root
causes and warrant distinct, actionable error messages.
Assisted-by: Claude Sonnet 4.6
- hack/bundle-gen.sh: replace `git checkout upstream/main` and `git checkout -B` with `git switch --detach` and `git switch -C` for unambiguous branch vs pathspec semantics. - hack/bundle-gen.sh: rm -rf the dummy-bundle output directory before cp -r so reruns for the same version replace it cleanly instead of nesting the new bundle inside the existing directory. - hack/version2.sh: pipe mapfile deduplication through `grep -v '^$'` to prevent empty arrays from producing a spurious empty-string entry when printf emits a blank line on zero-element input. Assisted-by: Claude Sonnet 4.6
|
@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. |
3 participants
Summary by CodeRabbit
Refactor
Chores
Bug Fixes