use multi layer push#55
Conversation
📝 WalkthroughWalkthroughAdds multi-layer OCI artifact support: push script can emit multi-layer manifests with per-layer annotations and decompressed-size sidecars; CLI pull detects multi-layer manifests and extracts sanitized per-layer files into a destination directory. Also wires an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Pull
participant Registry as OCI Registry
participant Manifest as Manifest Parser
participant Extractor as Extractor
participant FS as Filesystem
User->>CLI: pullOCIArtifact(source, dest)
CLI->>Registry: fetch manifest
Registry-->>Manifest: return manifest + annotations
Manifest->>Manifest: detect automotive.sdv.cloud.redhat.com/multi-layer
alt Multi-layer
CLI->>FS: create dest directory
loop per layer
Extractor->>Registry: fetch layer blob
Registry-->>Extractor: layer blob
Extractor->>FS: write layer file (sanitized name) via copyFile
end
else Single-file
Extractor->>Registry: fetch blob
Registry-->>Extractor: blob
Extractor->>FS: write/rename single file via copyFile
end
FS-->>User: artifact(s) extracted
sequenceDiagram
participant Script as push_artifact.sh
participant FS as Filesystem
participant ORAS as oras CLI
participant Registry as OCI Registry
Script->>FS: check for parts directory
alt Multi-layer Mode
Script->>FS: list part files and read .size sidecars
Script->>Script: build layer args and per-layer annotations (partition, decompressed-size)
Script->>ORAS: push with --annotation-file and --artifact-type
ORAS->>Registry: push layers + manifest (with annotations)
else Single-file Mode
Script->>FS: determine media-type / parts listing for tar
Script->>ORAS: push single blob with distro/target/arch annotations
ORAS->>Registry: push blob + manifest
end
Registry-->>Script: push complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
fixes #26 |
![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
🤖 Fix all issues with AI agents
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Line 138: The pipeline using tr on the string in push_artifact.sh is incorrect
because tr only maps single characters; replace the echo " ./${file_list}" | tr
',' '\n ./' usage with a proper multi-token splitter (e.g., use sed to replace
commas with a newline and the prefix "./ " or iterate over the comma-separated
$file_list with a loop and printf to print each as "./<file>") so that
function/variable references like file_list and the output formatting produce
one prefixed path per line instead of garbled characters.
- Around line 101-102: Update the per-layer annotation construction in
push_artifact.sh: instead of appending "--annotation
${filename}=automotive.sdv.cloud.redhat.com/partition=${partition_name}" to
layer_annotations, generate a temporary JSON file (e.g., annotations.json) that
maps each filename to an object with the key
"automotive.sdv.cloud.redhat.com/partition" and value partition_name (structure:
{ "filename": { "automotive.sdv.cloud.redhat.com/partition": "partition_value" }
}), ensure the JSON is written before invoking oras push, and then pass that
file to the oras push command using "--annotation-file annotations.json" (adjust
cleanup of the temp file accordingly); update references to filename,
partition_name, layer_annotations, and the oras push invocation to use the new
annotation file flow.
🧹 Nitpick comments (1)
cmd/caib/main.go (1)
841-868: Consider returning error from destination close for data integrity.When writing to a file,
Close()can fail if the OS hasn't flushed all data to disk (especially on network filesystems). Currently,dst.Close()errors are only logged. For critical data like disk images, this could silently produce corrupted files.♻️ Proposed fix to capture close error
func copyFile(srcPath, dstPath string) error { src, err := os.Open(srcPath) if err != nil { return fmt.Errorf("open source: %w", err) } defer func() { if err := src.Close(); err != nil { fmt.Fprintf(os.Stderr, "Warning: failed to close source file: %v\n", err) } }() dst, err := os.Create(dstPath) if err != nil { return fmt.Errorf("create destination: %w", err) } - defer func() { - if err := dst.Close(); err != nil { - fmt.Fprintf(os.Stderr, "Warning: failed to close destination file: %v\n", err) - } - }() if _, err := io.Copy(dst, src); err != nil { + dst.Close() return fmt.Errorf("copy data: %w", err) } - return nil + if err := dst.Close(); err != nil { + return fmt.Errorf("close destination: %w", err) + } + return nil }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 816-827: The filename taken from
layer.Annotations["org.opencontainers.image.title"] must be validated before
use: ensure you sanitize and reject absolute paths or any path containing "..",
separators, or zero bytes, and fall back to a safe generated name (e.g.,
layer-%d.bin) when invalid; perform this check right after reading filename and
before calling filepath.Join(destPath, filename), and only pass the
sanitized/safe basename to copyFile so extraction cannot escape destPath (update
the logic around filename, filepath.Join, and the copyFile call).
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 38-40: get_partition_name currently strips .simg/.raw/.img and
optional compression suffixes but misses the intermediate .tar (so names like
foo.simg.tar.gz remain foo.simg.tar). Update the sed/regex in get_partition_name
to accept an optional “.tar” between the base extension and the compression
extension (mirror what get_media_type recognizes) so that patterns like
.simg.tar.gz and .simg.tar.lz4 are fully removed; adjust the pattern used in
basename | sed -E 's/...//' inside get_partition_name accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 111-159: The loop can leave file_list/layer_args empty when no
partition files are present, producing invalid JSON and a dangling comma in
layer_annotations_json; after the for loop (before computing first_part or
writing annotations_file) add a guard that checks if [ -z "$file_list" ] (or [
-z "$layer_args" ]) and if so print a clear error to stderr (e.g., echo "No
partition files found in ${parts_dir}" >&2) and exit 1 to fail fast; update
references in the guard to the existing variables (file_list, layer_args,
layer_annotations_json) so the script never proceeds to build annotations_file
when no parts were collected.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 50-55: The current case for "*.lz4)" calls lz4 with the wrong flag
("lz4 -l") which is the legacy compress flag; change the invocation in the
"*.lz4)" branch to use the modern listing flag ("--list") and keep the existing
post-processing (grep/awk) to extract the decompressed size (noting it may be
empty if --content-size wasn't stored). Update the command invocation used in
the "*.lz4)" block so that the script uses "lz4 --list \"$file\"" (still
redirecting stderr and falling back to echo "") to correctly obtain file
information.
- Around line 50-55: In the *.lz4) case block replace the legacy lz4 -l
invocation with lz4 --list and extract the uncompressed size from the fifth
column (e.g. lz4 --list "$file" 2>/dev/null | awk '{print $5}') instead of using
grep/awk on the last field; keep the command existence check (command -v lz4)
and the error redirection/empty-string fallback (|| echo "") so the block
returns the size or "" if unavailable.
🧹 Nitpick comments (2)
internal/common/tasks/scripts/push_artifact.sh (2)
129-133: JSON construction may break with special characters in filenames.The JSON is built via string concatenation without escaping special characters. If a filename or partition name contains quotes (
"), backslashes (\), or control characters, the resulting JSON will be invalid.Consider using
jqto safely construct the JSON:♻️ Proposed safer JSON construction
- if [ -n "$uncompressed_size" ]; then - layer_annotations_json="${layer_annotations_json}\"${filename}\":{\"automotive.sdv.cloud.redhat.com/partition\":\"${partition_name}\",\"automotive.sdv.cloud.redhat.com/uncompressed-size\":\"${uncompressed_size}\"}" - else - layer_annotations_json="${layer_annotations_json}\"${filename}\":{\"automotive.sdv.cloud.redhat.com/partition\":\"${partition_name}\"}" - fi + # Build per-layer annotation safely using jq + if [ -n "$uncompressed_size" ]; then + layer_entry=$(jq -n --arg f "$filename" --arg p "$partition_name" --arg s "$uncompressed_size" \ + '{($f): {"automotive.sdv.cloud.redhat.com/partition": $p, "automotive.sdv.cloud.redhat.com/uncompressed-size": $s}}') + else + layer_entry=$(jq -n --arg f "$filename" --arg p "$partition_name" \ + '{($f): {"automotive.sdv.cloud.redhat.com/partition": $p}}') + fi + layer_annotations_json="${layer_annotations_json}${layer_entry}"
207-214: Tar content listing may fail with unusual filenames.The pipeline
tar -tf ... | ... | xargs -I{} basename {}can break if filenames contain newlines. Consider using-print0/-0options or a safer approach.However, since partition filenames in this context are typically simple alphanumeric names, this is a minor edge case.
💡 Safer alternative if needed
- file_list=$(tar -tf "${exportFile}" 2>/dev/null | grep -v '/$' | xargs -I{} basename {} | sort | tr '\n' ',' | sed 's/,$//') + file_list=$(tar -tf "${exportFile}" 2>/dev/null | grep -v '/$' | while read -r f; do basename "$f"; done | sort | tr '\n' ',' | sed 's/,$//')
| *.lz4) | ||
| if command -v lz4 >/dev/null 2>&1; then | ||
| lz4 -l "$file" 2>/dev/null | grep -i "decompressed" | awk '{print $NF}' || echo "" | ||
| else | ||
| echo "" | ||
| fi |
There was a problem hiding this comment.
Incorrect lz4 flag for getting file information.
The lz4 CLI has a --list command that provides "block type, checksum information, compressed and decompressed sizes (if present)." However, the script uses lz4 -l which is for legacy format compression, not listing file info.
🐛 Proposed fix
*.lz4)
- if command -v lz4 >/dev/null 2>&1; then
- lz4 -l "$file" 2>/dev/null | grep -i "decompressed" | awk '{print $NF}' || echo ""
- else
- echo ""
- fi
+ # lz4 --list shows frame info including decompressed size if stored
+ # Output format varies; try to extract from "Decompressed: <size>" or similar
+ if command -v lz4 >/dev/null 2>&1; then
+ lz4 --list "$file" 2>/dev/null | awk '/[Dd]ecompressed|[Uu]ncompressed/ {gsub(/,/,""); for(i=1;i<=NF;i++) if($i~/^[0-9]+$/) {print $i; exit}}' || echo ""
+ else
+ echo ""
+ fiNote: The uncompressed size is only available if it was stored in the frame header during compression (via --content-size flag). If not present, this will return empty.
🤖 Prompt for AI Agents
In `@internal/common/tasks/scripts/push_artifact.sh` around lines 50 - 55, The
current case for "*.lz4)" calls lz4 with the wrong flag ("lz4 -l") which is the
legacy compress flag; change the invocation in the "*.lz4)" branch to use the
modern listing flag ("--list") and keep the existing post-processing (grep/awk)
to extract the decompressed size (noting it may be empty if --content-size
wasn't stored). Update the command invocation used in the "*.lz4)" block so that
the script uses "lz4 --list \"$file\"" (still redirecting stderr and falling
back to echo "") to correctly obtain file information.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file and context
cat -n internal/common/tasks/scripts/push_artifact.sh | sed -n '40,65p'Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 958
🌐 Web query:
lz4 command -l flag documentation list file information
💡 Result:
In the official lz4 CLI, -l is not “list” — it means Legacy format (used e.g. for Linux kernel compression), and it’s not compatible with -m/-r. [1][2]
To list .lz4 file information, use the long option:
lz4 --list file.lz4— “List information about .lz4 files” (noted as limited to single-frame.lz4files in that manpage). [1][2]- Add
-vfor more detail (useful when inspecting multi-frame files). [3]
Example
lz4 --list mydata.lz4
lz4 --list -v mydata.lz4Sources: lz4(1) man page (Oracle / Solaris) [1], lz4(1) man page (openSUSE) [2], lz4 project option summary (“List mode”) [3].
🌐 Web query:
lz4 -l --list output format decompressed size
💡 Result:
lz4 --list prints a one-line summary per input file with columns (when not in verbose -v mode): Frames, Type, Block, Compressed, Uncompressed, Ratio, Filename. The “Compressed” and “Uncompressed” fields are human-readable sizes; “Ratio” is Compressed/Uncompressed * 100 (otherwise -). [1]
The “Uncompressed” (decompressed) size is only shown when the file’s LZ4 frame header contains the optional “Content Size” field; otherwise lz4 --list prints - for Uncompressed/Ratio. [1][2]
If you used -l (legacy format), --list can’t report the uncompressed size for those legacy frames (it prints -). [1]
Sources: [1] (lz4io.c: LZ4IO_displayCompressedFilesInfo() / LZ4IO_getCompressedFileInfo() output) https://pigweed.googlesource.com/third_party/github/lz4/lz4/+/refs/heads/upstream/threadpool_pthread/programs/lz4io.c, [2] (LZ4 frame format: Content Size is optional) https://android.googlesource.com/platform/external/lz4/+/HEAD/doc/lz4_Frame_format.md
🏁 Script executed:
# Check lz4 availability and help output
if command -v lz4 >/dev/null 2>&1; then
echo "=== lz4 help output ==="
lz4 --help 2>&1 | grep -A 5 "list"
echo ""
echo "=== lz4 man page snippet ==="
lz4 -h 2>&1 | head -60
else
echo "lz4 command not found"
fiRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 112
🌐 Web query:
lz4 --list output columns format "Uncompressed"
💡 Result:
In lz4 --list output, the Uncompressed column is the original (decoded) data size in bytes—i.e., how big the file will be after decompression. The CLI’s --list feature is specifically meant to show “compressed and decompressed sizes (if present)”. [1]
If the LZ4 frame doesn’t store the original content size (the default unless the file was created with --content-size), then --list can’t know it and will show it as unknown / not present (exact placeholder depends on the lz4 build/version). The man page notes that the header “includes original size” only when --[no-]content-size is enabled. [2]
Sources: [1] [2]
🏁 Script executed:
# Search for tests or usages of get_uncompressed_size function
rg "get_uncompressed_size" -A 3 -B 1Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 884
🌐 Web query:
lz4 --list output example columns order position uncompressed
💡 Result:
For the lz4 CLI, --list prints these columns (left → right): Frames, Type, Block, Compressed, Uncompressed, Ratio, Filename. The header is literally printed that way by the CLI. [1]
In verbose mode (for per-frame details), the columns are: Frame, Type, Block, Checksum, Compressed, Uncompressed, Ratio. [1]
Example (values are illustrative; the column order is the key part):
Frames Type Block Compressed Uncompressed Ratio Filename
1 LZ4F 4MB 12.3 MB 45.6 MB 27.0% data.lz4
Uncompressed is the 5th column in the default --list output. [1]
Sources
[1] https://android.googlesource.com/platform/external/lz4/+/master/programs/lz4io.c
🏁 Script executed:
# Look for test files related to push_artifact
find . -name "*test*" -o -name "*spec*" | grep -E "(push|artifact)" | head -20
rg "\.lz4" -l | head -10Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 199
Replace lz4 -l with lz4 --list to correctly retrieve uncompressed size.
The -l flag denotes legacy format, not list mode. Use lz4 --list instead, which outputs columns including the uncompressed size at position 5 (matching the xz parsing pattern on line 58):
Suggested fix
*.lz4)
if command -v lz4 >/dev/null 2>&1; then
- lz4 -l "$file" 2>/dev/null | grep -i "decompressed" | awk '{print $NF}' || echo ""
+ lz4 --list "$file" 2>/dev/null | awk 'NR==2 {print $5}' || echo ""
else
echo ""
fiNote: If an .lz4 file was not created with the optional content-size metadata, lz4 --list will display - and the function will return that value; consider fallback logic if needed.
📝 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.
| *.lz4) | |
| if command -v lz4 >/dev/null 2>&1; then | |
| lz4 -l "$file" 2>/dev/null | grep -i "decompressed" | awk '{print $NF}' || echo "" | |
| else | |
| echo "" | |
| fi | |
| *.lz4) | |
| if command -v lz4 >/dev/null 2>&1; then | |
| lz4 --list "$file" 2>/dev/null | awk 'NR==2 {print $5}' || echo "" | |
| else | |
| echo "" | |
| fi |
🤖 Prompt for AI Agents
In `@internal/common/tasks/scripts/push_artifact.sh` around lines 50 - 55, In the
*.lz4) case block replace the legacy lz4 -l invocation with lz4 --list and
extract the uncompressed size from the fifth column (e.g. lz4 --list "$file"
2>/dev/null | awk '{print $5}') instead of using grep/awk on the last field;
keep the command existence check (command -v lz4) and the error
redirection/empty-string fallback (|| echo "") so the block returns the size or
"" if unavailable.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 558-565: The .size sidecar is written regardless of compression
outcome; change the logic in build_image.sh so the call to echo
"$uncompressed_size" > "$parts_dir/${base}${EXT_FILE}.size" only runs if
compress_file "$item" "$parts_dir/${base}${EXT_FILE}" succeeded (check its exit
status/result). Locate the block that calls compress_file (using variables
parts_dir, base, EXT_FILE, uncompressed_size) and wrap the sidecar write in the
success branch (or use && to chain it) so failed compressions do not produce
.size metadata.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 139-144: The artifact type is being derived from a raw directory
listing (first_part=$(ls ./* | head -1)) which can accidentally pick up sidecar
files like .size; replace that logic to use the already-filtered file_list
variable as the source for the first artifact instead, and pass its basename
into get_artifact_type to set artifact_type (i.e., derive artifact_type from the
first entry of file_list rather than from ls output).
In `@internal/common/tasks/tasks.go`:
- Around line 54-58: The new ParamSpec named "arch" in tasks.go is required but
not supplied by the TaskRun in internal/controller/imagebuild/controller.go; fix
by either adding a Default value to the "arch" ParamSpec (Name: "arch") in the
Task Spec or by adding the "arch" Param to the TaskRun params when constructing
the TaskRun in the image build controller (use imageBuild.Spec.Architecture to
populate the param). Locate the ParamSpec with Name "arch" and add Default:
"<appropriate-arch>" or update the TaskRun creation code that builds params to
include a param entry for "arch" using imageBuild.Spec.Architecture so TaskRuns
always receive a value.
| # Record uncompressed size before compression (for OCI layer annotations) | ||
| uncompressed_size=$(stat -c%s "$item" 2>/dev/null || stat -f%z "$item" 2>/dev/null || echo "") | ||
| echo "Creating $parts_dir/${base}${EXT_FILE} (uncompressed: ${uncompressed_size:-unknown} bytes)" | ||
| compress_file "$item" "$parts_dir/${base}${EXT_FILE}" || echo "Failed to create $parts_dir/${base}${EXT_FILE}" | ||
| # Store uncompressed size in sidecar file for push_artifact.sh | ||
| if [ -n "$uncompressed_size" ]; then | ||
| echo "$uncompressed_size" > "$parts_dir/${base}${EXT_FILE}.size" | ||
| fi |
There was a problem hiding this comment.
Only write .size metadata when compression succeeds.
Right now the sidecar is created even if compression fails, which can leave misleading metadata. Gate the write on a successful compress.
Suggested fix
- compress_file "$item" "$parts_dir/${base}${EXT_FILE}" || echo "Failed to create $parts_dir/${base}${EXT_FILE}"
- # Store uncompressed size in sidecar file for push_artifact.sh
- if [ -n "$uncompressed_size" ]; then
- echo "$uncompressed_size" > "$parts_dir/${base}${EXT_FILE}.size"
- fi
+ if compress_file "$item" "$parts_dir/${base}${EXT_FILE}"; then
+ # Store uncompressed size in sidecar file for push_artifact.sh
+ if [ -n "$uncompressed_size" ]; then
+ echo "$uncompressed_size" > "$parts_dir/${base}${EXT_FILE}.size"
+ fi
+ else
+ echo "Failed to create $parts_dir/${base}${EXT_FILE}"
+ fi📝 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.
| # Record uncompressed size before compression (for OCI layer annotations) | |
| uncompressed_size=$(stat -c%s "$item" 2>/dev/null || stat -f%z "$item" 2>/dev/null || echo "") | |
| echo "Creating $parts_dir/${base}${EXT_FILE} (uncompressed: ${uncompressed_size:-unknown} bytes)" | |
| compress_file "$item" "$parts_dir/${base}${EXT_FILE}" || echo "Failed to create $parts_dir/${base}${EXT_FILE}" | |
| # Store uncompressed size in sidecar file for push_artifact.sh | |
| if [ -n "$uncompressed_size" ]; then | |
| echo "$uncompressed_size" > "$parts_dir/${base}${EXT_FILE}.size" | |
| fi | |
| # Record uncompressed size before compression (for OCI layer annotations) | |
| uncompressed_size=$(stat -c%s "$item" 2>/dev/null || stat -f%z "$item" 2>/dev/null || echo "") | |
| echo "Creating $parts_dir/${base}${EXT_FILE} (uncompressed: ${uncompressed_size:-unknown} bytes)" | |
| if compress_file "$item" "$parts_dir/${base}${EXT_FILE}"; then | |
| # Store uncompressed size in sidecar file for push_artifact.sh | |
| if [ -n "$uncompressed_size" ]; then | |
| echo "$uncompressed_size" > "$parts_dir/${base}${EXT_FILE}.size" | |
| fi | |
| else | |
| echo "Failed to create $parts_dir/${base}${EXT_FILE}" | |
| fi |
🤖 Prompt for AI Agents
In `@internal/common/tasks/scripts/build_image.sh` around lines 558 - 565, The
.size sidecar is written regardless of compression outcome; change the logic in
build_image.sh so the call to echo "$uncompressed_size" >
"$parts_dir/${base}${EXT_FILE}.size" only runs if compress_file "$item"
"$parts_dir/${base}${EXT_FILE}" succeeded (check its exit status/result). Locate
the block that calls compress_file (using variables parts_dir, base, EXT_FILE,
uncompressed_size) and wrap the sidecar write in the success branch (or use &&
to chain it) so failed compressions do not produce .size metadata.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 11-16: The current file-type case branch in push_artifact.sh maps
non-tar compressed filenames (*.gz, *.lz4, *.xz, *.zst|*.zstd) to
tar+compression media types; change those branches to emit a non-tar media type
(e.g., application/octet-stream or a specific vendor type such as
application/vnd.automotive.disk.raw+gzip) instead of
application/vnd.oci.image.layer.v1.tar+<compression>, and ensure the same
updated media type is used by the multi-layer partition push code path and the
single-file push fallback so partition.simg.gz and similar files are not
advertised as tar archives; alternatively, if the tar mapping is intentional,
add a clear comment in the case branches explaining why non-tar compressed files
must be labeled as tar+compression for OCI compatibility and reference the
multi-layer and single-file push logic that relies on it.
🧹 Nitpick comments (2)
internal/common/tasks/scripts/push_artifact.sh (2)
116-121: Consider escaping special characters in JSON construction.Variables like
${partition_name}and${filename}are interpolated directly into JSON strings. If a filename contains characters like",\, or newlines, the resulting JSON would be malformed.While this may be low risk if inputs are controlled (e.g., build system guarantees safe filenames), consider using
jqfor robust JSON construction if available, or at minimum document the filename constraints.♻️ Optional: Use jq for safer JSON building
# If jq is available, you could build JSON entries more safely: # jq -n --arg fn "$filename" --arg pn "$partition_name" \ # '{($fn): {"automotive.sdv.cloud.redhat.com/partition": $pn}}'
195-202: Tar content listing may fail with special filenames.Line 197 uses
xargs -I{} basename {}which can break with filenames containing spaces or special characters. Consider using null-delimited processing or a while-read loop for robustness:♻️ More robust alternative
- file_list=$(tar -tf "${exportFile}" 2>/dev/null | grep -v '/$' | xargs -I{} basename {} | sort | tr '\n' ',' | sed 's/,$//') + file_list=$(tar -tf "${exportFile}" 2>/dev/null | grep -v '/$' | while IFS= read -r f; do basename "$f"; done | sort | tr '\n' ',' | sed 's/,$//')
bennyz
force-pushed
the
oras-multi
branch
2 times, most recently
from
c1ec368 to
cdee8a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/imagebuild/controller.go (1)
333-340:⚠️ Potential issue | 🟡 MinorAdd safe default or validation for
imageBuild.Spec.Architecturebefore using in Tekton params and node selectors.
imageBuild.Spec.Architectureis optional (markedomitempty) with no validation or defaulting logic, yet it's used directly in three places: Tekton task params (lines 338, 678) and node selector values (line 559). Unlikedistro,target, andmodefields which use safe helper methods (GetDistro(), etc.),Architecturelacks this protection. An empty architecture value passed to node selectors withNodeSelectorOpInoperator could fail pod scheduling. Add aGetArchitecture()helper method, CRD validation to require the field, or defensive checks before construction of params.Also applies to: 674-680, 559
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 925-940: The loop that extracts layers can produce duplicate
sanitized filenames so copyFile may silently overwrite; modify the extraction in
the manifest.Layers loop to track seen names (e.g., map[string]int or
map[string]bool) keyed by the sanitized filename returned by sanitizeFilename,
and before calling copyFile check the map: if the name already exists, return a
descriptive error (include the sanitized name, current layer index i, and
previous index/digest/annotation) or alternatively disambiguate by appending a
suffix (e.g., "-1", "-2") to the basename; update the map when a name is
accepted and then call copyFile with the unique filename so overwrites cannot
occur silently.
🧹 Nitpick comments (1)
cmd/caib/main.go (1)
954-1001: Prefer rejecting path separators instead of silently collapsing to basename.Using
filepath.Basewhen the input contains path separators can create collisions or unintentionally change filenames. Consider treating such inputs as invalid and falling back tolayer-N.binfor deterministic mapping.♻️ Suggested adjustment
- base := filepath.Base(filename) - if base != filename { - fmt.Fprintf(os.Stderr, "Warning: layer %d filename contains path separators, using basename: %s\n", layerIndex, base) - filename = base - } + base := filepath.Base(filename) + if base != filename { + fmt.Fprintf(os.Stderr, "Warning: layer %d filename contains path separators, using fallback\n", layerIndex) + return fallback + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 124-136: The JSON built in push_artifact.sh via string
concatenation (variables layer_annotations_json, base_annotations,
partition_name, filename, decompressed_size) is vulnerable to injection if
filename or partition_name contain quotes or backslashes; update the code to
construct the per-layer JSON safely by using jq to build objects (e.g., create
an object with keys partition and org.opencontainers.image.title and optional
decompressed-size, then merge into layer_annotations_json) or, if jq isn't
available, ensure filename and partition_name are escaped for " and \ and
newlines before interpolation; replace the current concatenation that appends
"\"${filename}\":{${base_annotations}...}" with a jq-based or properly escaped
construction to guarantee valid JSON.
🧹 Nitpick comments (1)
cmd/caib/main.go (1)
1027-1054: Consider cleanup on copy failure.The
copyFilefunction works correctly for the happy path. A minor robustness improvement would be to remove the partially-written destination file ifio.Copyfails, preventing leftover corrupt files.♻️ Optional: Clean up partial file on error
func copyFile(srcPath, dstPath string) error { src, err := os.Open(srcPath) if err != nil { return fmt.Errorf("open source: %w", err) } defer func() { if err := src.Close(); err != nil { fmt.Fprintf(os.Stderr, "Warning: failed to close source file: %v\n", err) } }() dst, err := os.Create(dstPath) if err != nil { return fmt.Errorf("create destination: %w", err) } - defer func() { - if err := dst.Close(); err != nil { - fmt.Fprintf(os.Stderr, "Warning: failed to close destination file: %v\n", err) - } - }() - if _, err := io.Copy(dst, src); err != nil { + if _, err := io.Copy(dst, src); err != nil { + dst.Close() + os.Remove(dstPath) // Clean up partial file return fmt.Errorf("copy data: %w", err) } + if err := dst.Close(); err != nil { + return fmt.Errorf("close destination: %w", err) + } + return nil }
and add annotation to OCI image Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
1 participant
and add annotation to OCI image
Summary by CodeRabbit