NO-ISSUE: Add inspect-catalog helper - #2942
Open
pacevedom wants to merge 4 commits into
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pacevedom: This pull request explicitly references no 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
size/L
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom 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
WalkthroughAdds ChangesCatalog inspection utility
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Operator
participant inspect_catalog
participant skopeo
participant CatalogLayers
participant jq
Operator->>inspect_catalog: provide catalog image and filters
inspect_catalog->>skopeo: inspect manifest and copy image
skopeo-->>CatalogLayers: provide image layers
CatalogLayers-->>inspect_catalog: provide catalog.json files
inspect_catalog->>jq: build and filter bundle metadata
inspect_catalog->>skopeo: fetch vcs-ref labels when enabled
inspect_catalog-->>Operator: return JSON or formatted report
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 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: 4
🤖 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/inspect-catalog.sh`:
- Around line 1-2: Add lightweight functional coverage for the filtering and
parsing logic in hack/inspect-catalog.sh, exercising resolve_arch_digest and
build_output with fixture inputs that include host:port stripping and
version-regex cases. Integrate the bats or shell test with the repository’s
existing verification workflow and ensure all tests pass.
- Around line 92-95: Update the external-command handling in the catalog
inspection flow, especially the skopeo inspect invocation near raw and the
skopeo copy invocation, so failures retain and surface stderr before set -e
terminates the script. Preserve successful stdout parsing and ensure
authentication, network, or invalid-reference errors are emitted with useful
command context instead of being suppressed.
- Around line 87-109: Update the base-image derivation in resolve_arch_digest()
so tag removal preserves registry ports, stripping only the tag from the final
path segment of the image reference. Keep the existing digest output format
unchanged, ensuring references such as localhost:5000/catalog:v1 resolve to
localhost:5000/catalog@${digest}.
- Around line 162-166: Update the bundle version filter in the jq pipeline to
use test() for regex-style matching against .name, while preserving the exact
$bundle_version comparison and the existing suffix match.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b201e08-60f3-498d-9021-d472e900fe3c
📒 Files selected for processing (1)
hack/inspect-catalog.sh
Comment on lines
+1
to
+2
| #!/bin/bash | ||
| set -euo pipefail |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
No test coverage for the filtering/parsing logic.
Two of the bugs flagged above (host:port stripping, version-regex condition) are exactly the kind a small functional test would catch. As per coding guidelines, "Contributions should include tests as needed, and all tests must pass before submission." Consider a lightweight bats/shell test (or a make verify-integrated check) exercising resolve_arch_digest and build_output against fixture inputs.
🤖 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/inspect-catalog.sh` around lines 1 - 2, Add lightweight functional
coverage for the filtering and parsing logic in hack/inspect-catalog.sh,
exercising resolve_arch_digest and build_output with fixture inputs that include
host:port stripping and version-regex cases. Integrate the bats or shell test
with the repository’s existing verification workflow and ensure all tests pass.
Source: Coding guidelines
Comment on lines
+87
to
+109
| resolve_arch_digest() { | ||
| local image="$1" | ||
| local arch="$2" | ||
|
|
||
| local raw | ||
| raw=$(skopeo inspect --raw "docker://${image}" 2>/dev/null) | ||
|
|
||
| local media_type | ||
| media_type=$(echo "${raw}" | jq -r '.mediaType // .schemaVersion' 2>/dev/null) | ||
|
|
||
| case "${media_type}" in | ||
| *index*|*list*) | ||
| local digest | ||
| digest=$(echo "${raw}" | jq -r \ | ||
| --arg arch "${arch}" \ | ||
| '.manifests[] | select(.platform.architecture == $arch) | .digest' 2>/dev/null | head -1) | ||
| if [[ -z "${digest}" ]]; then | ||
| echo "Error: no manifest found for architecture ${arch}" >&2 | ||
| exit 1 | ||
| fi | ||
| local base | ||
| base=$(echo "${image}" | sed 's/@sha256:.*//; s/:.*$//') | ||
| echo "${base}@${digest}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Reproduce the bug: sed strips the registry port instead of the tag
echo "localhost:5000/catalog:v1" | sed 's/@sha256:.*//; s/:.*$//'
# Expected: localhost:5000/catalog -- Actual: localhostRepository: openshift/lvm-operator
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant script section and demonstrate the sed behavior for several image refs.
sed -n '80,115p' hack/inspect-catalog.sh
echo "--- sed port/tag handling ---"
for ref in \
registry.example.com:5000/catalog:v1 \
quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:abc \
localhost:5000/catalog:v1 \
catalog:v1 \
localhost:5000/catalog \
app:v1 \
a/b/c:v2; do
printf '%s -> %s\n' "$ref" "$(printf '%s' "$ref" | sed 's/@sha256:.*//; s/:.*$/')"
doneRepository: openshift/lvm-operator
Length of output: 1751
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- sed behavior using bash command substitution ---"
for ref in \
registry.example.com:5000/catalog:v1 \
registry.example.com:5000/catalog \
quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:abc \
localhost:5000/catalog:v1 \
catalog:v1 \
localhost:5000/catalog \
app:v1 \
a/b/c:v2; do
printf '%s -> %s\n' "$ref" "$(printf '%s' "$ref" | sed 's/@sha256:.*//; s/:.*$/')"
doneRepository: openshift/lvm-operator
Length of output: 906
Handle registry ports when stripping image tags.
In resolve_arch_digest(), sed 's/:.*$//' strips everything after the first :, so localhost:5000/catalog:v1 becomes localhost instead of localhost:5000/catalog. Use the last path segment to remove only the tag, otherwise the resolved ${base}@${digest} reference fed to skopeo copy will be invalid for ported registries.
🤖 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/inspect-catalog.sh` around lines 87 - 109, Update the base-image
derivation in resolve_arch_digest() so tag removal preserves registry ports,
stripping only the tag from the final path segment of the image reference. Keep
the existing digest output format unchanged, ensuring references such as
localhost:5000/catalog:v1 resolve to localhost:5000/catalog@${digest}.
Comment on lines
+92
to
+95
| raw=$(skopeo inspect --raw "docker://${image}" 2>/dev/null) | ||
|
|
||
| local media_type | ||
| media_type=$(echo "${raw}" | jq -r '.mediaType // .schemaVersion' 2>/dev/null) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
External command failures are completely silent.
Line 92 discards skopeo inspect --raw stderr, and line 123 discards both stdout and stderr from skopeo copy. Combined with set -e, any failure (auth, network, bad ref) exits the script immediately with zero diagnostic output — a rough experience for a tool whose whole purpose is inspection/troubleshooting.
🩹 Proposed fix: surface errors before exiting
local raw
- raw=$(skopeo inspect --raw "docker://${image}" 2>/dev/null)
+ raw=$(skopeo inspect --raw "docker://${image}") || {
+ echo "Error: failed to inspect ${image}" >&2
+ exit 1
+ }-skopeo copy "docker://${RESOLVED_IMAGE}" "dir://${IMGDIR}" >/dev/null 2>&1
+if ! skopeo copy "docker://${RESOLVED_IMAGE}" "dir://${IMGDIR}" >/dev/null; then
+ echo "Error: failed to pull image ${RESOLVED_IMAGE}" >&2
+ exit 1
+fiAlso applies to: 123-123
🤖 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/inspect-catalog.sh` around lines 92 - 95, Update the external-command
handling in the catalog inspection flow, especially the skopeo inspect
invocation near raw and the skopeo copy invocation, so failures retain and
surface stderr before set -e terminates the script. Preserve successful stdout
parsing and ensure authentication, network, or invalid-reference errors are
emitted with useful command context instead of being suppressed.
Comment on lines
+162
to
+166
| [.[] | select(.schema == "olm.bundle") | | ||
| select(if $pkg != "" then .package == $pkg else true end) | | ||
| ((.properties // [])[] | select(.type == "olm.package") | .value.version) as $bundle_version | | ||
| select(if $ver != "" then ($bundle_version == $ver or .name == (".*v" + $ver) or (.name | endswith("v" + $ver))) else true end) | | ||
| . as $bundle | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm equality vs regex-test semantics in jq
echo '{"name":"lvms-operator.v4.19.3"}' | jq '.name == (".*v" + "4.19.3")' # false
echo '{"name":"lvms-operator.v4.19.3"}' | jq '.name | test(".*v" + "4.19.3")' # trueRepository: openshift/lvm-operator
Length of output: 171
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '145,175p' hack/inspect-catalog.sh
printf '\njq matches in file:\n'
rg -n "\.name ==" hack/inspect-catalog.shRepository: openshift/lvm-operator
Length of output: 1681
Use a regex test for name-based version matches.
.name == ("v" + $ver) still compares against the literal string ".*v4.19.3", so regex-style matching never fires. Use test() instead if matching versions in bundle names is intended.
♻️ Proposed fix
- select(if $ver != "" then ($bundle_version == $ver or .name == (".*v" + $ver) or (.name | endswith("v" + $ver))) else true end) |
+ select(if $ver != "" then ($bundle_version == $ver or (.name | test(".*v" + $ver)) or (.name | endswith("v" + $ver))) else true end) |📝 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.
Suggested change
| [.[] | select(.schema == "olm.bundle") | | |
| select(if $pkg != "" then .package == $pkg else true end) | | |
| ((.properties // [])[] | select(.type == "olm.package") | .value.version) as $bundle_version | | |
| select(if $ver != "" then ($bundle_version == $ver or .name == (".*v" + $ver) or (.name | endswith("v" + $ver))) else true end) | | |
| . as $bundle | | |
| [.[] | select(.schema == "olm.bundle") | | |
| select(if $pkg != "" then .package == $pkg else true end) | | |
| ((.properties // [])[] | select(.type == "olm.package") | .value.version) as $bundle_version | | |
| select(if $ver != "" then ($bundle_version == $ver or (.name | test(".*v" + $ver)) or (.name | endswith("v" + $ver))) else true end) | | |
| . as $bundle | |
🤖 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/inspect-catalog.sh` around lines 162 - 166, Update the bundle version
filter in the jq pipeline to use test() for regex-style matching against .name,
while preserving the exact $bundle_version comparison and the existing suffix
match.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2942 +/- ##
==========================================
- Coverage 54.72% 54.23% -0.49%
==========================================
Files 54 53 -1
Lines 4225 4239 +14
==========================================
- Hits 2312 2299 -13
- Misses 1732 1758 +26
- Partials 181 182 +1 🚀 New features to boost your workflow:
|
Contributor
Author
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/inspect-catalog.sh`:
- Around line 103-106: Update the manifest digest extraction pipeline in
hack/inspect-catalog.sh to convert an unmatched architecture result from JSON
null to an empty value, using first // empty or an equivalent null check before
the existing empty validation. Ensure the subsequent image-reference
construction cannot emit `@null` for tag images.
- Around line 308-320: The catalog rendering in inspect-catalog.sh is only using
the first channel and first upgrade entry for each bundle, so update the jq flow
around the group_by(.channels[0]) block to expand each bundle across all of its
channels instead of collapsing to .channels[0]. Then, within the per-channel
output, select the matching upgrade_info item by channel before calling
format_upgrade, so the bundle image and upgrade metadata stay aligned for every
channel.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8196ab08-d2a8-4279-8369-a836556f3879
📒 Files selected for processing (1)
hack/inspect-catalog.sh
Comment on lines
+103
to
+106
| '[.manifests[] | | ||
| select(.platform.architecture == $arch) | | ||
| .digest] | first' \ | ||
| 2>/dev/null) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)hack/inspect-catalog\.sh$|(^|/)Makefile$|(^|/)README' || true
echo
echo "== inspect-catalog relevant lines =="
if [ -f hack/inspect-catalog.sh ]; then
nl -ba hack/inspect-catalog.sh | sed -n '80,120p'
fi
echo
echo "== jq first behavior =="
jq -nr '[] | first' || true
jq -nr '[' | jq '. as {msg: .} | empty' || true
printf 'jq 2>/dev/null null:'; jq -nr '[] | first | tostring' 2>/dev/null || true
echo
echo "== search null check patterns =="
rg -n "first|manifest\.digest|jq -n|r|`@null`|null" hack/inspect-catalog.sh 2>/dev/null || trueRepository: openshift/lvm-operator
Length of output: 357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect-catalog relevant lines =="
if [ -f hack/inspect-catalog.sh ]; then
awk '{printf "%6d\t%s\n", NR, $0}' hack/inspect-catalog.sh | sed -n '80,120p'
fi
echo
echo "== jq first behavior =="
jq -nr '[] | first' || true
printf '%s\n' 'jq 2>/dev/null null:'
jq -nr '[] | first | tostring' 2>/dev/null || true
echo
echo "== search related patterns =="
grep -nE 'first|manifest\.digest|jq|`@null`|null|MISSING|architecture|digest' hack/inspect-catalog.sh || trueRepository: openshift/lvm-operator
Length of output: 3401
Reject an unmatched architecture before building the image reference.
When no manifest matches arch, first outputs JSON null, and the empty check at line 107 does not reject it. Add first // empty or reject null before line 107 to avoid emitting @null for a tag image.
🤖 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/inspect-catalog.sh` around lines 103 - 106, Update the manifest digest
extraction pipeline in hack/inspect-catalog.sh to convert an unmatched
architecture result from JSON null to an empty value, using first // empty or an
equivalent null check before the existing empty validation. Ensure the
subsequent image-reference construction cannot emit `@null` for tag images.
Comment on lines
+308
to
+320
| group_by(.package)[] | | ||
| .[0].package as $pkg | | ||
| "Package: \($pkg)", | ||
| (group_by(.channels[0])[] | | ||
| .[0].channels[0] as $chan | | ||
| "", | ||
| " Channel: \($chan)", | ||
| " \("-" * 70)", | ||
| (sort_by(.version | | ||
| split(".") | map(tonumber? // 0))[] | | ||
| .image as $bundle_img | | ||
| (.upgrade_info[0] | | ||
| format_upgrade) as $ug | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and relevant lines"
wc -l hack/inspect-catalog.sh || true
sed -n '280,340p' hack/inspect-catalog.sh
echo
echo "Search for format_upgrade definition and channel metadata usage"
rg -n "format_upgrade|upgrade_info|group_by|flatten|channels" hack/inspect-catalog.shRepository: openshift/lvm-operator
Length of output: 2018
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect channel metadata construction"
sed -n '130,205p' hack/inspect-catalog.sh
echo
echo "Read full jq script without helper definitions"
sed -n '1,230p' hack/inspect-catalog.sh
echo
echo "Search for generate jq script"
rg -n "jq --null-input|jq -n|channel_entries|upgrade_info|channels:" .Repository: openshift/lvm-operator
Length of output: 8965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Run read-only jq probe against a synthetic multi-channel catalog"
python3 - <<'PY'
import json
catalog = {
"schema": "olm.package", "name": "pkg", "defaultChannel": "default",
"channels": [{"name": "alpha", "head": "bundle-v1.0.0"}, {"name": "beta", "head": "bundle-v1.0.0"}]
}
channels = {
"schema": "olm.channel", "name": "alpha", "entries": [
{"name": "bundle-v1.0.0", "replaces": "alpha-0.1.0"}
]
}
channels2 = {
"schema": "olm.channel", "name": "beta", "entries": [
{"name": "bundle-v1.0.0", "replaces": "beta-0.1.0"}
]
}
bundle = {
"schema": "olm.bundle", "name": "bundle-v1.0.0", "package": "pkg",
"image": "sha256:aaaa", "relatedImages": [],
"properties": [{"type": "olm.package", "value": {"package": "pkg", "version": "1.0.0"}}]
}
print(json.dumps([catalog, channels, channels2, bundle], indent=2))
PY
jq -n --slurpfile all <(python3 - <<'PY'
import json
catalog = {
"schema": "olm.package", "name": "pkg", "defaultChannel": "default",
"channels": [{"name": "alpha", "head": "bundle-v1.0.0"}, {"name": "beta", "head": "bundle-v1.0.0"}]
}
channels = {
"schema": "olm.channel", "name": "alpha", "entries": [
{"name": "bundle-v1.0.0", "replaces": "alpha-0.1.0"}
]
}
channels2 = {
"schema": "olm.channel", "name": "beta", "entries": [
{"name": "bundle-v1.0.0", "replaces": "beta-0.1.0"}
]
}
bundle = {
"schema": "olm.bundle", "name": "bundle-v1.0.0", "package": "pkg",
"image": "sha256:aaaa", "relatedImages": [],
"properties": [{"type": "olm.package", "value": {"package": "pkg", "version": "1.0.0"}}]
}
print(json.dumps([catalog, channels, channels2, bundle]))
PY
) '
def channel_entries:
[.[] | select(.schema == "olm.channel") |
. as $ch | .entries[]? |
{channel: $ch.name, bundle: .name, replaces: .replaces,
skips: (.skips // []), skipRange: .skipRange}];
def get_version:
[(.properties // [])[] |
select(.type == "olm.package") | .value.version
] | first // "";
def short_sha: .;
def short_commit: .;
def strip_prefix: sub("^v"; "") | sub("^"; "");
. as $all |
channel_entries as $channels |
[.[] | select(.schema == "olm.bundle") |
get_version as $bv |
. as $bundle |
{
package: .package,
name: .name,
version: $bv,
image: .image,
channels: ([($channels[] |
select(.bundle == $bundle.name) | .channel)] | unique),
upgrade_info: [($channels[] |
select(.bundle == $bundle.name) |
{channel: .channel, replaces: .replaces,
skips: .skips, skipRange: .skipRange})],
relatedImages: [(.relatedImages // [])[] |
{name: .name, image: .image}]
}
] | sort_by(.package, .name)
'Repository: openshift/lvm-operator
Length of output: 1213
Render every channel for a multi-channel bundle.
The catalog query stores one channels array and one upgrade_info array per bundle, so grouping by .channels[0] only publishes the first channel. upgrade_info[0] can also show upgrade metadata for a different channel. Expand each bundle per channel and select the upgrade_info entry whose channel matches the current channel.
🤖 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/inspect-catalog.sh` around lines 308 - 320, The catalog rendering in
inspect-catalog.sh is only using the first channel and first upgrade entry for
each bundle, so update the jq flow around the group_by(.channels[0]) block to
expand each bundle across all of its channels instead of collapsing to
.channels[0]. Then, within the per-channel output, select the matching
upgrade_info item by channel before calling format_upgrade, so the bundle image
and upgrade metadata stay aligned for every channel.
Contributor
|
@pacevedom: all tests passed! 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. |
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
3 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.
Summary by CodeRabbit
Summary by CodeRabbit