internal registry fixes#94
Conversation
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
📝 WalkthroughWalkthroughResolve external registry route in build listing and translate image URLs when service-account auth is used; make default-partitions annotation conditional in artifact push script and remove multi-layer pull guidance; extend imagebuild reconciler to read registry credentials from a SecretRef and create a flash-oci-auth secret when appropriate. Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant K8sAPI
participant RegistrySecret
participant FlashProcess
Reconciler->>K8sAPI: Get ImageBuild CR
Reconciler->>K8sAPI: Read SecretRef (if present)
K8sAPI-->>Reconciler: Secret data (REGISTRY_USERNAME, REGISTRY_PASSWORD)
alt both username and password present
Reconciler->>K8sAPI: Create `flash-oci-auth` Secret (username/password, owner refs)
K8sAPI-->>Reconciler: Secret created
Reconciler->>FlashProcess: Provide flash-oci-auth secret reference
else partial or missing creds
Reconciler->>Reconciler: Log and skip secret creation
Reconciler->>FlashProcess: Continue with SA-token or other auth path
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/controller/imagebuild/controller.go`:
- Around line 538-577: The creation of the flash-OCI auth Secret (ociSecret /
flashOCIAuthSecretName) via r.Create can return AlreadyExists during
reconciliation retries and should be guarded; update the block that calls
r.Create(ctx, ociSecret) to detect and ignore metav1.IsAlreadyExists (or
apierrors.IsAlreadyExists) errors so reconciliation continues (treat existing
secret as success), and return real errors for other cases—apply the same
AlreadyExists handling pattern used elsewhere (e.g., the SA-token secret
creation) to the flash-oci-auth secret creation for imageBuild.
- Around line 549-551: The code currently checks REGISTRY_USERNAME and
REGISTRY_PASSWORD bytes and silently skips creating the flash-oci-auth secret
when only one is present; update the logic in the function where registrySecret,
regUser and regPass are used (the block that creates the flash-oci-auth secret)
to detect partial credentials (one present and the other empty) and either log a
clear warning via the same logger used elsewhere or return an explicit error
instead of no-oping; reference the registrySecret, regUser, regPass and the
secret-creation branch to implement the check and emit a helpful message that
indicates which key is missing and that credential creation was skipped.
🧹 Nitpick comments (2)
internal/controller/imagebuild/controller.go (1)
553-576: Significant duplication with SA-token secret creation block (lines 514–537).The secret structure (labels, owner refs, type, data keys) is nearly identical between the two branches. Consider extracting a helper like
createFlashOCIAuthSecret(name, namespace, imageBuild, username, password)to reduce drift risk.internal/common/tasks/scripts/push_artifact.sh (1)
279-290: Verify JSON validity whendefault_partitions_annotationis non-empty.The interpolated annotation fragment (line 286) relies on
default_partitions_annotationstarting with,\nand thearchline having no trailing comma. This works, but is fragile if anyone later adds a field afterarchwithout adjusting. A briefcat "$annotations_file" | python3 -m json.tool > /dev/nullsanity check could guard against silent breakage, though this is optional given the current simplicity.
it shows how to pull from registry service, instead of route Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
dde4043 to
81e1900
Compare
Summary by CodeRabbit