refactor: extract OCI artifact contract into shared spec#295
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR centralizes OCI annotation/media/referrer metadata in an embedded spec (internal/common/oci), exposes Go helpers and deterministic shell-vars, injects those variables into embedded task scripts, and refactors Go inspect and shell push/build logic to use the spec-derived constants and lookups. ChangesOCI Specification Centralization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (2)
internal/common/oci/spec.go (1)
136-142: 💤 Low valuePotential shell variable name collision risk.
The
shellVarNamefunction transforms both-,., andto_, which could cause collisions if spec.json contains keys that differ only in these characters (e.g.,foo-barandfoo.barwould both becomeFOO_BAR). While this doesn't appear to be an issue with the current spec.json content, consider adding a collision check during init() or documenting this constraint.🛡️ Optional collision detection in init()
func init() { if err := json.Unmarshal(specJSON, &spec); err != nil { panic(fmt.Sprintf("oci: failed to parse spec.json: %v", err)) } + // Validate no shell variable name collisions + seen := make(map[string]string) + checkCollision := func(key, category string) { + normalized := shellVarName(key) + if existing, ok := seen[normalized]; ok && existing != key { + panic(fmt.Sprintf("oci: shell variable name collision: %q and %q both map to %s", existing, key, normalized)) + } + seen[normalized] = key + } + for _, k := range spec.Annotations.Manifest.Required { + checkCollision(k, "manifest.required") + } + // ... repeat for other key collections }🤖 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 `@internal/common/oci/spec.go` around lines 136 - 142, The shellVarName function maps "-", ".", and " " to "_" (in shellVarName), which can produce identical names for distinct spec keys; add a startup collision detection in init() that computes shellVarName(key) for every key from spec.json and fails (or logs a clear error) if any two different original keys map to the same transformed name, referencing shellVarName and the init function to locate where to validate and include both conflicting original keys in the error message so authors can correct or be informed of the constraint.cmd/caib/inspectcmd/inspect.go (1)
314-319: 💤 Low valueThe
annotationDisplayLabelsmap is out of sync with the spec.
AllManifestAnnotationKeys()returns unprefixed short names from the spec. The map currently has 11 entries but the spec defines 14 annotation keys (3 required + 11 optional). The missing keysparts,multi-layer, anddefault-partitionswill silently skip from provenance output at line 316 whenlabel == "". UpdateannotationDisplayLabelsto include all spec keys, or source the labels from the spec to prevent future drift.🤖 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 `@cmd/caib/inspectcmd/inspect.go` around lines 314 - 319, The annotationDisplayLabels map is missing three spec keys so AllManifestAnnotationKeys() yields entries that are skipped when label == ""; update annotationDisplayLabels to include the spec keys "parts", "multi-layer", and "default-partitions" with their corresponding display strings (or instead populate labels dynamically from the spec metadata) so that the loop in inspect.go (using ociSpec.AllManifestAnnotationKeys() and annotations[ociSpec.AnnotationKey(key)]) does not silently drop those annotations; ensure the keys match the unprefixed short names returned by AllManifestAnnotationKeys() and keep naming consistent with AnnotationKey lookups.
🤖 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 `@cmd/caib/inspectcmd/inspect.go`:
- Line 373: Replace the hardcoded filename lookups in buildRebuildCommand
(currently using referrerByFile("manifest.aib.yml") and
referrerByFile("build-sources.tar.gz")) with lookups derived from the spec – use
ociSpec.ReferrerTypes (or the existing referrer accessors) to obtain the correct
referrer keys and then query referrerTypes; update the hasManifest/hasSources
assignment to derive the lookup keys from ociSpec.ReferrerTypes (or a dedicated
helper method) instead of literal filenames so the logic remains correct if spec
filenames change (see buildRebuildCommand, referrerByFile, and
ociSpec.ReferrerTypes).
---
Nitpick comments:
In `@cmd/caib/inspectcmd/inspect.go`:
- Around line 314-319: The annotationDisplayLabels map is missing three spec
keys so AllManifestAnnotationKeys() yields entries that are skipped when label
== ""; update annotationDisplayLabels to include the spec keys "parts",
"multi-layer", and "default-partitions" with their corresponding display strings
(or instead populate labels dynamically from the spec metadata) so that the loop
in inspect.go (using ociSpec.AllManifestAnnotationKeys() and
annotations[ociSpec.AnnotationKey(key)]) does not silently drop those
annotations; ensure the keys match the unprefixed short names returned by
AllManifestAnnotationKeys() and keep naming consistent with AnnotationKey
lookups.
In `@internal/common/oci/spec.go`:
- Around line 136-142: The shellVarName function maps "-", ".", and " " to "_"
(in shellVarName), which can produce identical names for distinct spec keys; add
a startup collision detection in init() that computes shellVarName(key) for
every key from spec.json and fails (or logs a clear error) if any two different
original keys map to the same transformed name, referencing shellVarName and the
init function to locate where to validate and include both conflicting original
keys in the error message so authors can correct or be informed of the
constraint.
🪄 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: Pro
Run ID: 639d5701-7fa1-43c9-8f5c-ee0cc8b5bf2d
📒 Files selected for processing (11)
cmd/caib/common/oci_artifact.gocmd/caib/inspectcmd/inspect.gocmd/caib/inspectcmd/inspect_test.gointernal/common/oci/spec.gointernal/common/oci/spec.jsoninternal/common/oci/spec_test.gointernal/common/tasks/oci_contract_test.gointernal/common/tasks/scripts.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/scripts/sealed_operation.sh
|
/e2e-test-all |
| func shellVarName(key string) string { | ||
| return strings.ToUpper(strings.NewReplacer( | ||
| "-", "_", | ||
| ".", "_", | ||
| " ", "_", | ||
| ).Replace(key)) | ||
| } |
There was a problem hiding this comment.
This method converts -, ., and spaces to _ so different keys could produce the same variable name (e.g. foo-bar and foo.bar both become FOO_BAR). Doesn't affect current keys, but adding a duplicate check in init() would catch future collisions early.
| "aib-extra-args": "AIB Extra Args", | ||
| "export-format": "Export Format", | ||
| "aib-command": "AIB Command", | ||
| } |
There was a problem hiding this comment.
annotationDisplayLabels map has 11 entries but the spec defines 14 annotation keys, the three missing keys (parts, multi-layer, default-partitions) are silently skipped.
There was a problem hiding this comment.
the 3 others are not for provenance, but for tools (FLS)
Define media types, annotation keys, and referrer types in a single JSON contract file (internal/common/oci/spec.json) consumed by Go via //go:embed and by shell scripts via generated variables. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
worktree-oci-artifact-contract
branch
from
268670d to
69c022e
Compare
|
@ambient-code please review |
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good refactoring — centralizing the OCI constants into a single spec.json is a solid approach that eliminates string duplication across Go and shell. The contract test in oci_contract_test.go is a nice guardrail against regression. A few observations below, mostly around silent-failure modes in the lookup helpers.
|
|
||
| // ReferrerArtifactTypeByLabel returns the artifact type string for a given display label. | ||
| func (s *Spec) ReferrerArtifactTypeByLabel(label string) string { | ||
| for _, r := range s.ReferrerTypes { |
There was a problem hiding this comment.
ReferrerArtifactTypeByLabel returns "" on a miss with no signal that the label was invalid. Since the callers in inspect.go use compile-time-known string literals ("AIB Manifest", "Build Sources"), a typo or spec.json label rename will silently evaluate referrerTypes[""] to false, hiding the bug.
Consider either panicking on unknown labels (these are effectively internal constants, not user input):
func (s *Spec) ReferrerArtifactTypeByLabel(label string) string {
for _, r := range s.ReferrerTypes {
if r.Label == label {
return r.ArtifactType
}
}
panic(fmt.Sprintf("oci: unknown referrer label %q", label))
}Or switching callers to use the artifact type directly from ociSpec.ReferrerTypes — the label indirection adds a fragile string coupling that the spec was meant to eliminate.
| if aib_cmd: a[k_aib_cmd] = aib_cmd | ||
| if task_bundle: a[k_task_bundle] = task_bundle | ||
| if custom_defs: a[k_custom_defs] = custom_defs | ||
| if extra_args: a[k_extra_args] = extra_args |
There was a problem hiding this comment.
The Python blocks now accept OCI annotation keys as positional args (sys.argv[14:28]). Since the OCI_ANN_* variables are already injected as shell variables (and therefore available as environment variables to child processes), the Python code could use os.environ["OCI_ANN_DISTRO"] directly instead of threading 14 positional args through the command line.
This would:
- Make the arg/index mapping less fragile (no risk of off-by-one when adding a new annotation)
- Keep the Python heredoc self-documenting
- Remove the need to maintain the long
$OCI_ANN_...continuation lines
Not a blocker, but worth considering for the second Python block as well (~line 430).
| *.qcow2.gz|*.qcow2.lz4|*.qcow2.xz|*.qcow2) echo "$OCI_MEDIA_DISK_QCOW2" ;; | ||
| *.raw.gz|*.raw.lz4|*.raw.xz|*.raw|*.img.gz|*.img.lz4|*.img.xz|*.img) echo "$OCI_MEDIA_DISK_RAW" ;; | ||
| *) echo "$OCI_MEDIA_OCTETSTREAM" ;; | ||
| esac |
There was a problem hiding this comment.
Pre-existing, but: get_artifact_type handles *.simg.gz and *.simg.lz4 but not *.simg.xz, while get_media_type above covers all three compression variants for every disk format. Might be worth adding *.simg.xz to the pattern:
| esac | |
| *.simg.gz|*.simg.lz4|*.simg.xz|*.simg) echo "$OCI_MEDIA_DISK_SIMG" ;; |
maboras-rh
left a comment
maboras-rh
left a comment
There was a problem hiding this comment.
lgtm - solid & structured refactor for oci contract into spec.json
beside of that, I added 2 short comments.
2 participants
Define media types, annotation keys, and referrer types in a single JSON contract file (internal/common/oci/spec.json) consumed by Go via //go:embed and by shell scripts via generated variables.
Summary
Related Issues
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit