fix golangci lint#32
Conversation
📝 WalkthroughWalkthroughThis PR introduces a CI/CD workflow via GitHub Actions for linting and testing, refactors the build API with new types and public methods for client configuration and file uploads, expands catalog handlers with improved error handling and constants, enhances multiple controllers with phase management and logging infrastructure, and adds package documentation and linting directives throughout. Changes
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
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/buildapi/server.go (2)
1728-1731: Critical: Same shell injection risk in streamArtifactByFilename.The
filenameparameter has the same insufficient validation asfileinstreamArtifactPart. Apply consistent input validation usingvalidateInput.🔒 Proposed fix
- if strings.Contains(filename, "/") || strings.Contains(filename, "..") || strings.TrimSpace(filename) == "" { + if err := validateInput(filename, "artifact filename", 255, false, "/", ".."); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid file name"}) return }
1429-1436: Critical: Shell command injection vulnerability in artifact streaming endpoints.The
fileparameter instreamArtifactPart(line 1433) and thefilenameparameter instreamArtifactByFilename(line 1728) are validated only for/and.., but are then interpolated into shell commands via double-quoted strings. Double quotes do not prevent command substitution—an attacker can inject characters like$(),`, or escape the quotes to execute arbitrary commands. ThevalidateInputfunction already exists in the codebase (line 696) and comprehensively blocks shell metacharacters; it should be used here.🔒 Proposed fix
For
streamArtifactPart:func (a *APIServer) streamArtifactPart(c *gin.Context, name, file string) { namespace := resolveNamespace() ctx := c.Request.Context() - if strings.Contains(file, "/") || strings.Contains(file, "..") || strings.TrimSpace(file) == "" { + if err := validateInput(file, "artifact file", 255, false, "/", ".."); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid file name"}) return }For
streamArtifactByFilename:func (a *APIServer) streamArtifactByFilename(c *gin.Context, name, filename string) { namespace := resolveNamespace() ctx := c.Request.Context() - if strings.Contains(filename, "/") || strings.Contains(filename, "..") || strings.TrimSpace(filename) == "" { + if err := validateInput(filename, "artifact file", 255, false, "/", ".."); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid file name"}) return }cmd/caib/main.go (2)
1469-1520: Security: Tar extraction vulnerable to path traversal (zip slip).The
extractTarfunction joinsdestDirwithhdr.Namewithout validating that the resulting path stays withindestDir. A malicious tar archive with entries like../../../etc/passwdcould write outside the intended directory.🔒 Recommended fix to prevent path traversal
func extractTar(tarPath, destDir string) error { + absDestDir, err := filepath.Abs(destDir) + if err != nil { + return fmt.Errorf("resolve dest dir: %w", err) + } + f, err := os.Open(tarPath) if err != nil { return err } defer func() { if err := f.Close(); err != nil { fmt.Fprintf(os.Stderr, "Warning: failed to close tar file: %v\n", err) } }() var r io.Reader = f if strings.HasSuffix(strings.ToLower(tarPath), ".gz") { gr, gzErr := gzip.NewReader(f) if gzErr == nil { defer func() { if err := gr.Close(); err != nil { fmt.Fprintf(os.Stderr, "Warning: failed to close gzip reader: %v\n", err) } }() r = gr } } tr := tar.NewReader(r) for { hdr, err := tr.Next() if err == io.EOF { break } if err != nil { return err } - targetPath := filepath.Join(destDir, hdr.Name) + // Prevent path traversal attacks + targetPath := filepath.Join(destDir, hdr.Name) + absTargetPath, err := filepath.Abs(targetPath) + if err != nil { + return fmt.Errorf("resolve target path: %w", err) + } + if !strings.HasPrefix(absTargetPath, absDestDir+string(os.PathSeparator)) && absTargetPath != absDestDir { + return fmt.Errorf("illegal path in archive: %s", hdr.Name) + } + switch hdr.Typeflag {
1521-1527: Security: Symlink target not validated in tar extraction.The symlink creation at line 1525 uses
hdr.Linknamedirectly without validation. A malicious archive could create a symlink pointing outside the destination directory (e.g.,hdr.Linkname = "/etc/passwd"), which could be exploited in subsequent operations.🔒 Recommended fix to validate symlink targets
case tar.TypeSymlink: if err := os.MkdirAll(filepath.Dir(targetPath), 0o755); err != nil { return err } + // Validate symlink target stays within destDir + linkTarget := hdr.Linkname + if !filepath.IsAbs(linkTarget) { + linkTarget = filepath.Join(filepath.Dir(targetPath), linkTarget) + } + absLinkTarget, err := filepath.Abs(linkTarget) + if err != nil { + return fmt.Errorf("resolve symlink target: %w", err) + } + if !strings.HasPrefix(absLinkTarget, absDestDir+string(os.PathSeparator)) && absLinkTarget != absDestDir { + return fmt.Errorf("illegal symlink target in archive: %s -> %s", hdr.Name, hdr.Linkname) + } if err := os.Symlink(hdr.Linkname, targetPath); err != nil && !os.IsExist(err) { return err }
🤖 Fix all issues with AI agents
In `@api/v1alpha1/catalogimage_types.go`:
- Around line 27-31: The kubebuilder Pattern marker is broken across multiple
comment lines starting with the token "+kubebuilder:validation:Pattern=";
collapse the full regex into a single-line kubebuilder marker (or remove the
marker if you don't want strict validation) so controller-gen can parse it
correctly—locate the multi-line Pattern comments around the CatalogImage type
(the "+kubebuilder:validation:Pattern=" marker) and replace them with one valid
single-line Pattern comment containing the complete regex (or delete the
marker).
In `@cmd/caib/catalog/add.go`:
- Around line 62-67: The current MarkFlagRequired error branches return nil
which can cause downstream nil dereferences; instead fail-fast by replacing
those "return nil" branches with a deterministic exit that logs the error (e.g.,
call log.Fatalf or fmt.Fprintf(os.Stderr, ...) followed by os.Exit(1)) and
include the actual error text; specifically update the two error checks around
cmd.MarkFlagRequired("architecture") and cmd.MarkFlagRequired("distro") to log a
clear message like "failed to mark required flag '<flag>': %v" and exit rather
than returning nil so the command construction never yields a nil cmd.
In `@internal/controller/catalogimage/catalogimage_controller.go`:
- Around line 52-57: Multiple +kubebuilder:rbac marker comments are split across
lines which breaks controller-gen; collapse each into a single-line comment so
each +kubebuilder:rbac entry is complete on one line. Specifically, replace the
multi-line markers for resources "catalogimages", "catalogimages/status", and
"catalogimages/finalizers" in catalogimage_controller.go with three single-line
comments of the form: //
+kubebuilder:rbac:groups=automotive.sdv.cloud.redhat.com,resources=<resource>,verbs=<verbs>.
In `@internal/controller/imagebuild/controller.go`:
- Around line 86-102: The kubebuilder RBAC markers in controller.go currently
use backslash continuation across multiple comment lines (e.g., the "//
+kubebuilder:rbac:groups=security.openshift.io,resources=securitycontextconstraints,\\"
and similar markers for rbac.authorization.k8s.io and tekton.dev), which is
unsupported; replace each multi-line marker with one or more standard
single-line "// +kubebuilder:rbac:..." comments (either a single long line per
rule with semicolon-separated verbs or separate marker comments for each
resource/verb) so that every RBAC rule appears on its own single-line
kubebuilder marker.
In `@internal/controller/operatorconfig/controller.go`:
- Around line 68-70: The RBAC kubebuilder markers in controller.go are split
across lines and malformed; replace each split marker with complete single-line
markers so controller-gen can parse them — specifically ensure you have a full
marker for operatorconfigs including verbs
(get;list;watch;create;update;patch;delete) and a full marker for
operatorconfigs/status including verbs (get;update;patch); update both
occurrences (the pair currently around the operatorconfigs and the pair around
operatorconfigs/status) to be complete single-line comments matching kubebuilder
syntax.
🧹 Nitpick comments (15)
internal/common/tasks/tasks.go (1)
24-25: Consider digest pinning or verifying the image tag.Line 25 introduces a mutable tag (
:1.0.0). For reproducibility and supply-chain safety, consider pinning to a digest or confirming the tag’s immutability and availability in Quay..github/workflows/lint.yml (1)
42-53: Redundant lint execution.The "Verify no lint issues" step runs
make linta second time after it was already executed in "Run golangci-lint". This doubles the lint execution time. The verification logic can be combined with the initial lint run.♻️ Suggested consolidation
- name: Install golangci-lint run: make golangci-lint - - name: Run golangci-lint - run: make lint - - - name: Verify no lint issues + - name: Run golangci-lint and verify no issues run: | - ISSUES=$(make lint 2>&1 | grep -E "^[0-9]+ issues:" | cut -d' ' -f1 || echo "0") - if [ "$ISSUES" != "0" ]; then - echo "❌ Found $ISSUES lint issues - PR cannot be merged" + if ! make lint; then + echo "❌ Lint issues found - PR cannot be merged" exit 1 - else - echo "✅ No lint issues found - great job!" fi + echo "✅ No lint issues found - great job!"test/e2e/e2e_test.go (1)
462-485: Replace customcontainswithstrings.Contains.This reimplements
strings.Containsfrom the standard library. The standard library version is more efficient (uses optimized search algorithms) and is already imported via thestringspackage on line 24.Additionally, the behavior differs:
strings.Contains("", "")returnstrue, but this implementation returnsfalsefor empty inputs.♻️ Replace with standard library
Remove the custom functions and use
strings.Containsdirectly:-func contains(s, substr string) bool { - if len(s) == 0 || len(substr) == 0 { - return false - } - if s == substr { - return true - } - if len(s) < len(substr) { - return false - } - // Check prefix, suffix, or middle - return s[:len(substr)] == substr || - s[len(s)-len(substr):] == substr || - containsMiddle(s, substr) -} - -func containsMiddle(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}Then update call sites (lines 127, 133) to use
strings.Contains:- if !contains(tasks, "build-automotive-image") { + if !strings.Contains(tasks, "build-automotive-image") {- if !contains(tasks, "push-artifact-registry") { + if !strings.Contains(tasks, "push-artifact-registry") {internal/buildapi/client/client.go (1)
173-216: Review the goroutine structure for potential edge cases.The upload goroutine structure is functional but has a subtle consideration: when the inner goroutine calls
pw.CloseWithError(err)on error, the outer defers will still attempt to closemwandpw. Whileio.Pipehandles double-close gracefully,mw.Close()afterpw.CloseWithError()may log spurious warnings.Consider closing
mwbefore error propagation in the inner goroutine to avoid redundant close attempts.internal/buildapi/server.go (3)
1383-1393: Inconsistent quoting in shell commands.Shell commands use double quotes in some places (line 1492) and single quotes in others (lines 1630, 1795). Additionally, the shell command at lines 1383-1385 is complex and harder to audit. Consider extracting shell command construction into a helper that consistently escapes or validates paths.
Also applies to: 1492-1492, 1630-1630, 1795-1795
987-1010: Inconsistent logging: Useslog.Printfinstead ofa.log.The warning logs for owner reference failures use the standard library
log.Printfwhile the rest of the server uses the structureda.loglogger. This breaks observability consistency.♻️ Proposed fix to use structured logging
if err := setConfigMapOwnerRef(ctx, k8sClient, namespace, cfgName, imageBuild); err != nil { - log.Printf( - "WARNING: failed to set owner reference on ConfigMap %s: %v (cleanup may require manual intervention)", - cfgName, err, - ) + a.log.Error(err, "failed to set owner reference on ConfigMap, cleanup may require manual intervention", + "configMap", cfgName) } if envSecretRef != "" { if err := setSecretOwnerRef(ctx, k8sClient, namespace, envSecretRef, imageBuild); err != nil { - log.Printf( - "WARNING: failed to set owner reference on registry secret %s: %v "+ - "(cleanup may require manual intervention)", - envSecretRef, err, - ) + a.log.Error(err, "failed to set owner reference on registry secret, cleanup may require manual intervention", + "secret", envSecretRef) } } if pushSecretName != "" { if err := setSecretOwnerRef(ctx, k8sClient, namespace, pushSecretName, imageBuild); err != nil { - log.Printf( - "WARNING: failed to set owner reference on push secret %s: %v "+ - "(cleanup may require manual intervention)", - pushSecretName, err, - ) + a.log.Error(err, "failed to set owner reference on push secret, cleanup may require manual intervention", + "secret", pushSecretName) } }
703-716: Minor: Appending to module-level slice.Line 705 appends to the module-level
shellMetacharsslice. While safe due to howappendworks (returns a new backing array if capacity is exceeded), this pattern can be confusing. Consider creating a new slice explicitly.♻️ More explicit slice construction
func validateInput(value, fieldName string, maxLen int, allowEmpty bool, extraChars ...string) error { if value == "" { if allowEmpty { return nil } return fmt.Errorf("%s is required", fieldName) } // Combine shell metacharacters with any additional blocked characters - blockedChars := append(shellMetachars, extraChars...) + blockedChars := make([]string, 0, len(shellMetachars)+len(extraChars)) + blockedChars = append(blockedChars, shellMetachars...) + blockedChars = append(blockedChars, extraChars...) for _, char := range blockedChars {internal/buildapi/types.go (1)
100-108: ParseMode silently accepts invalid mode values.The function defaults to
ModeBootconly when the input is empty. A non-empty but invalid value like"invalid"would be accepted and returned without validation against the known mode constants.♻️ Proposed fix to validate mode values
// ParseMode parses a mode string, defaulting to bootc if empty. func ParseMode(s string) (Mode, error) { m := Mode(s) if !m.IsValid() { // Default to bootc if not specified return ModeBootc, nil } + switch m { + case ModeBootc, ModeImage, ModePackage, ModeDisk: + return m, nil + default: + return "", fmt.Errorf("invalid mode: %s (must be one of: bootc, image, package, disk)", s) + } - return m, nil }internal/controller/image/controller.go (1)
19-22: Consider adding aphaseVerifyingconstant for full consistency.
You’ve centralizedAvailable/Unavailable, butVerifyingremains a literal in several places. A small constant keeps phases uniform and typo-safe.♻️ Suggested refactor
const ( phaseAvailable = "Available" phaseUnavailable = "Unavailable" + phaseVerifying = "Verifying" ) switch image.Status.Phase { case "": return r.handleInitialState(ctx, image) -case "Verifying": +case phaseVerifying: return r.handleVerifyingState(ctx, image) - if err := r.updateStatus(ctx, image, "Verifying", "Starting image location verification"); err != nil { + if err := r.updateStatus(ctx, image, phaseVerifying, "Starting image location verification"); err != nil { - if err := r.updateStatus(ctx, image, "Verifying", "Re-verifying image location"); err != nil { + if err := r.updateStatus(ctx, image, phaseVerifying, "Re-verifying image location"); err != nil { - if err := r.updateStatus(ctx, image, "Verifying", "Retrying image location verification"); err != nil { + if err := r.updateStatus(ctx, image, phaseVerifying, "Retrying image location verification"); err != nil { - Reason: "Verifying", + Reason: phaseVerifying,internal/controller/imagebuild/controller.go (2)
502-504: Variable shadowing:nsNameis redeclared.The
nsNamevariable declared at line 472 is shadowed by a new declaration at line 503. While this doesn't cause a bug here (both have the same value), it's unnecessary and could confuse readers or cause issues if the outer value were different.♻️ Suggested fix
if imageBuild.Status.PVCName == "" { workspacePVCName, err := r.getOrCreateWorkspacePVC(ctx, imageBuild) if err != nil { return err } fresh := &automotivev1alpha1.ImageBuild{} - nsName := types.NamespacedName{Name: imageBuild.Name, Namespace: imageBuild.Namespace} - if err := r.Get(ctx, nsName, fresh); err != nil { + if err := r.Get(ctx, nsName, fresh); err != nil { return fmt.Errorf("failed to get fresh ImageBuild: %w", err) }
984-986: Variable shadowing:nsNameis redeclared again.Similar to the earlier instance,
nsNameat line 925 is shadowed by a redeclaration at line 985. Consider reusing the existing variable.♻️ Suggested fix
freshBuild := &automotivev1alpha1.ImageBuild{} - nsName := types.NamespacedName{Name: latestImageBuild.Name, Namespace: latestImageBuild.Namespace} - if err := r.Get(ctx, nsName, freshBuild); err != nil { + freshNsName := types.NamespacedName{Name: latestImageBuild.Name, Namespace: latestImageBuild.Namespace} + if err := r.Get(ctx, freshNsName, freshBuild); err != nil {cmd/caib/catalog/get.go (1)
80-84: Inconsistent warning output destination.This file writes the warning to stdout via
fmt.Printf, while other catalog commands (remove.go, publish.go, verify.go) write to stderr viafmt.Fprintf(os.Stderr, ...). Warnings should typically go to stderr to avoid polluting stdout output, especially for CLI tools that might pipe output.♻️ Suggested fix
defer func() { if err := resp.Body.Close(); err != nil { - fmt.Printf("Warning: failed to close response body: %v\n", err) + fmt.Fprintf(os.Stderr, "Warning: failed to close response body: %v\n", err) } }()cmd/caib/main.go (3)
346-347: Inconsistent error handling for MarkFlagRequired.Lines 346-347 use
_ = buildDevCmd.MarkFlagRequired(...)to discard errors, while line 279-281 uses proper error handling with warnings. Consider applying consistent handling across allMarkFlagRequiredcalls.♻️ Suggested consistent error handling
- _ = buildDevCmd.MarkFlagRequired("mode") - _ = buildDevCmd.MarkFlagRequired("format") + if err := buildDevCmd.MarkFlagRequired("mode"); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to mark 'mode' flag as required: %v\n", err) + } + if err := buildDevCmd.MarkFlagRequired("format"); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to mark 'format' flag as required: %v\n", err) + }
1337-1347: Potential issue:extractServerFilenamedoesn't handle quoted filenames with semicolons.The current parsing assumes
filename=is followed only by the filename. RFC 6266 allows parameters likefilename="file.txt"; filename*=UTF-8''file.txt. The current implementation would include trailing parameters in the filename.For most servers this is fine, but consider a more robust parser if you expect varied
Content-Dispositionformats.♻️ More robust filename extraction
func extractServerFilename(resp *http.Response) string { cd := resp.Header.Get("Content-Disposition") if cd == "" { return "" } if i := strings.Index(cd, "filename="); i >= 0 { - return strings.Trim(cd[i+9:], "\" ") + name := cd[i+9:] + // Handle quoted filenames and strip trailing parameters + if strings.HasPrefix(name, "\"") { + if end := strings.Index(name[1:], "\""); end >= 0 { + return name[1 : end+1] + } + } + // Unquoted: take until semicolon or end + if semi := strings.Index(name, ";"); semi >= 0 { + name = name[:semi] + } + return strings.TrimSpace(name) } return "" }
1349-1382: ReviewhandleArtifactDownloadresource cleanup ordering.The function closes
resp.Bodyexplicitly at line 1372 after download, but ifdownloadWithProgressfails (line 1366-1370), the body is not closed before returning. While the caller's deferred close would handle this in a retry loop,handleArtifactDownloadalso closes the body on success, creating inconsistent ownership.Consider clarifying ownership: either always let the caller close the body, or always close it in this function via defer.
♻️ Consistent body ownership with defer
func handleArtifactDownload(resp *http.Response, outDir, userFilename, name string) error { + defer func() { + if err := resp.Body.Close(); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to close response body: %v\n", err) + } + }() + contentType := resp.Header.Get("Content-Type") serverFilename := extractServerFilename(resp) compression := strings.TrimSpace(resp.Header.Get("X-AIB-Compression")) filename := resolveFilename(userFilename, serverFilename, compression, name+".artifact") printArtifactMetadata(resp) outPath := filepath.Join(outDir, filename) tmp := outPath + ".partial" f, err := os.Create(tmp) if err != nil { - _ = resp.Body.Close() return err } if err := downloadWithProgress(resp, f); err != nil { _ = f.Close() _ = os.Remove(tmp) return err } - _ = resp.Body.Close() if err := f.Close(); err != nil { return fmt.Errorf("failed to close file: %w", err) }
bennyz
force-pushed
the
fix-golangci
branch
4 times, most recently
from
baa846f to
3f59ebe
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
cmd/caib/main.go (2)
127-132: Handle registries without dots to avoid wrong credential targeting.Line 129 only recognizes hosts containing a dot as registries. References like
localhost:5000/fooorregistry:5000/foowill incorrectly fall back todocker.io, which can break push/pull and credential targeting.🔧 Proposed fix
- if len(parts) > 0 && strings.Contains(parts[0], ".") { - return parts[0] - } - return "docker.io" + if len(parts) > 0 { + host := parts[0] + if strings.Contains(host, ".") || strings.Contains(host, ":") || host == "localhost" { + return host + } + } + return "docker.io"
1491-1527: Block tar path traversal during extraction.Line 1500 uses
filepath.Joinwithout validatinghdr.Name, allowing../or absolute paths to write outsidedestDir(“tar slip”). Add a clean path check before processing each entry.🔐 Proposed fix
tr := tar.NewReader(r) + cleanDest := filepath.Clean(destDir) for { hdr, err := tr.Next() if err == io.EOF { break @@ - targetPath := filepath.Join(destDir, hdr.Name) + targetPath := filepath.Join(destDir, hdr.Name) + cleanPath := filepath.Clean(targetPath) + if cleanPath != cleanDest && !strings.HasPrefix(cleanPath, cleanDest+string(os.PathSeparator)) { + return fmt.Errorf("tar entry outside destination: %s", hdr.Name) + } switch hdr.Typeflag {test/e2e/e2e_test.go (1)
462-485: Replace customcontainswithstrings.Contains.This custom implementation duplicates
strings.Containsfrom the standard library. The stdlib version is optimized (uses assembly on most platforms) and handles edge cases correctly. The custom implementation also has a subtle bug:contains("", "")returnsfalse, butstrings.Contains("", "")returnstrueper the Go spec.♻️ Suggested fix
-func contains(s, substr string) bool { - if len(s) == 0 || len(substr) == 0 { - return false - } - if s == substr { - return true - } - if len(s) < len(substr) { - return false - } - // Check prefix, suffix, or middle - return s[:len(substr)] == substr || - s[len(s)-len(substr):] == substr || - containsMiddle(s, substr) -} - -func containsMiddle(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -} +// Use strings.Contains directly at call sites instead of this wrapperThen update the call sites (lines 127, 133) to use
strings.Contains:if !strings.Contains(tasks, "build-automotive-image") {internal/buildapi/server.go (1)
1267-1283: Avoid deferring temp-file cleanup inside the upload loop.Defers inside the loop keep every temp file and descriptor open until the handler returns. With many parts, this can exhaust file descriptors and memory. Clean up per iteration instead.
🛠️ Suggested refactor (per-part cleanup)
- tmpName := tmp.Name() - defer func() { - if err := tmp.Close(); err != nil { - fmt.Fprintf(os.Stderr, "Warning: failed to close temp file: %v\n", err) - } - }() - defer func() { - if err := os.Remove(tmpName); err != nil { - fmt.Fprintf(os.Stderr, "Warning: failed to remove temp file: %v\n", err) - } - }() + tmpName := tmp.Name() + cleanup := func() { + if err := tmp.Close(); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to close temp file: %v\n", err) + } + if err := os.Remove(tmpName); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to remove temp file: %v\n", err) + } + } ... - if err := copyFileToPod( + if err := copyFileToPod( restCfg, namespace, uploadPod.Name, uploadPod.Spec.Containers[0].Name, tmpName, destPath, ); err != nil { + cleanup() c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("stream to pod failed: %v", err)}) return } + cleanup()internal/controller/catalogimage/catalogimage_controller.go (1)
306-342: Reset the Verified condition when transitioning to Unavailable/Failed.
Right now,CatalogImageConditionVerifiedcan remain true from a prior success, which can mislead status consumers once the image becomes unavailable/failed.🛠️ Proposed fix
func (r *CatalogImageReconciler) transitionToUnavailable( ctx context.Context, catalogImage *automotivev1alpha1.CatalogImage, reason, message string, ) (ctrl.Result, error) { r.setCondition(catalogImage, automotivev1alpha1.CatalogImageConditionAvailable, metav1.ConditionFalse, reason, message) + r.setCondition(catalogImage, automotivev1alpha1.CatalogImageConditionVerified, metav1.ConditionFalse, reason, message) r.setCondition(catalogImage, automotivev1alpha1.CatalogImageConditionReady, metav1.ConditionFalse, reason, message) catalogImage.Status.Phase = automotivev1alpha1.CatalogImagePhaseUnavailable catalogImage.Status.ObservedGeneration = catalogImage.Generation @@ func (r *CatalogImageReconciler) transitionToFailed( ctx context.Context, catalogImage *automotivev1alpha1.CatalogImage, reason, message string, ) (ctrl.Result, error) { r.setCondition(catalogImage, automotivev1alpha1.CatalogImageConditionAvailable, metav1.ConditionFalse, reason, message) + r.setCondition(catalogImage, automotivev1alpha1.CatalogImageConditionVerified, metav1.ConditionFalse, reason, message) r.setCondition(catalogImage, automotivev1alpha1.CatalogImageConditionReady, metav1.ConditionFalse, reason, message)internal/controller/operatorconfig/controller.go (1)
270-277: Use theannotationKeyvariable consistently.Line 273 defines
annotationKeyfor the comparison, but line 277 hardcodes the same string again. Use the variable for consistency and to avoid drift.🔧 Suggested fix
buildAPIAnnotation := `{"kind":"OAuthRedirectReference","apiVersion":"v1",` + `"reference":{"kind":"Route","name":"ado-build-api"}}` annotationKey := "serviceaccounts.openshift.io/oauth-redirectreference.buildapi" if sa.Annotations[annotationKey] == buildAPIAnnotation { return nil // Already set } - sa.Annotations["serviceaccounts.openshift.io/oauth-redirectreference.buildapi"] = buildAPIAnnotation + sa.Annotations[annotationKey] = buildAPIAnnotation
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 1350-1375: In handleArtifactDownload, ensure resp.Body is closed
on all return paths by adding defer resp.Body.Close() immediately after entering
the function (near the start of handleArtifactDownload) and remove the manual
closes of resp.Body (the two `_ = resp.Body.Close()` calls); keep explicit
f.Close() and os.Remove(tmp) handling around downloadWithProgress and final file
close, and avoid double-closing resp.Body so connections aren’t leaked when
downloadWithProgress returns early.
In `@config/crd/bases/automotive.sdv.cloud.redhat.com_catalogimages.yaml`:
- Around line 145-152: The CRD's pattern for the registryUrl property was
mangled to a single backslash; replace the invalid pattern value under the
registryUrl `pattern` field with the full combined regex (remove the Go string
concatenation artifacts like `+` and ensure the regex alternatives are a single
valid YAML string) or regenerate the CRD from the kubebuilder marker
(kubebuilder:validation:Pattern in api/v1alpha1/catalogimage_types.go) so
controller-gen emits the correct regex; ensure the final YAML contains a single
quoted regex string that matches the three alternatives shown in the description
and not a lone backslash.
In `@internal/buildapi/server.go`:
- Around line 1382-1394: The code constructs shellCmd using unvalidated
artifactFileName and then passes it to sh -c for pod exec (see partsDir,
shellCmd, and the PodExecOptions usage); fix by removing sh -c where possible
and/or sanitizing inputs: implement a filename whitelist/validation function
(e.g., safeFilename) that rejects any characters like quotes, semicolons,
backticks, pipes, newlines, slashes or path traversal, apply it to
artifactFileName (and other variables: file, base, filename used elsewhere)
before building any command, and build exec calls using exec args array without
sh -c (pass commands and parameters as separate strings) to avoid shell
interpretation; ensure the same validation/approach is applied to other handlers
referenced (sizeCmd and the other exec call sites).
In `@test/utils/utils.go`:
- Line 26: Update the Ginkgo dot-import in utils.go to match other tests: remove
`staticcheck` from the nolint directive and use `//nolint:revive // Dot import
is standard for Ginkgo` so the line with the dot import `.
"github.com/onsi/ginkgo/v2"` only suppresses `revive` and includes the standard
explanatory comment.
🧹 Nitpick comments (5)
internal/buildapi/types.go (1)
64-108: ValidateModevalues inParseMode.Right now any non-empty value is accepted, which can propagate invalid modes downstream. Consider rejecting unknown modes early.
♻️ Suggested change
func ParseMode(s string) (Mode, error) { m := Mode(s) if !m.IsValid() { // Default to bootc if not specified return ModeBootc, nil } - return m, nil + switch m { + case ModeBootc, ModeImage, ModePackage, ModeDisk: + return m, nil + default: + return "", fmt.Errorf("invalid mode: %s", m) + } }cmd/caib/catalog/get.go (1)
43-115: Route warnings to stderr to keep output clean.The new parsing error handling is great. One small tweak: emit the close warning to stderr so JSON/YAML output on stdout stays clean.
🔧 Suggested tweak
- if err := resp.Body.Close(); err != nil { - fmt.Printf("Warning: failed to close response body: %v\n", err) - } + if err := resp.Body.Close(); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to close response body: %v\n", err) + }internal/controller/imagebuild/controller.go (3)
467-468: VariablensNameis shadowed in inner scope at line 498.The variable
nsNameis declared at line 467, but then re-declared with:=at line 498-499 within the same function. While Go allows this due to block scoping, it can lead to confusion. Consider using=instead of:=at line 498, or use a different variable name.🔧 Suggested fix
if imageBuild.Status.PVCName == "" { workspacePVCName, err := r.getOrCreateWorkspacePVC(ctx, imageBuild) if err != nil { return err } fresh := &automotivev1alpha1.ImageBuild{} - nsName := types.NamespacedName{Name: imageBuild.Name, Namespace: imageBuild.Namespace} - if err := r.Get(ctx, nsName, fresh); err != nil { + if err := r.Get(ctx, types.NamespacedName{Name: imageBuild.Name, Namespace: imageBuild.Namespace}, fresh); err != nil { return fmt.Errorf("failed to get fresh ImageBuild: %w", err) }
980-981: VariablensNameshadows outer scope variable.Similar to line 498,
nsNameis re-declared here with:=when updating artifact URL, shadowing the earlier declaration at line 920.🔧 Suggested fix
freshBuild := &automotivev1alpha1.ImageBuild{} - nsName := types.NamespacedName{Name: latestImageBuild.Name, Namespace: latestImageBuild.Namespace} - if err := r.Get(ctx, nsName, freshBuild); err != nil { + if err := r.Get(ctx, types.NamespacedName{Name: latestImageBuild.Name, Namespace: latestImageBuild.Namespace}, freshBuild); err != nil { log.Error(err, "Failed to get fresh ImageBuild for URL update") return ctrl.Result{RequeueAfter: 5 * time.Second}, nil }
28-33: Consolidate duplicate namespace constants for consistency.The
OperatorNamespaceconstant ininternal/controller/imagebuild/controller.go:29is exported (capitalized), while the identical constantoperatorNamespaceininternal/controller/operatorconfig/controller.go:25is unexported (lowercase). Both define"automotive-dev-operator-system"identically. Extract this to a shared constants package or ensure consistent naming across both controllers.
| func handleArtifactDownload(resp *http.Response, outDir, userFilename, name string) error { | ||
| contentType := resp.Header.Get("Content-Type") | ||
| serverFilename := extractServerFilename(resp) | ||
| compression := strings.TrimSpace(resp.Header.Get("X-AIB-Compression")) | ||
| filename := resolveFilename(userFilename, serverFilename, compression, name+".artifact") | ||
|
|
||
| printArtifactMetadata(resp) | ||
|
|
||
| outPath := filepath.Join(outDir, filename) | ||
| tmp := outPath + ".partial" | ||
| f, err := os.Create(tmp) | ||
| if err != nil { | ||
| _ = resp.Body.Close() | ||
| return err | ||
| } | ||
|
|
||
| if err := downloadWithProgress(resp, f); err != nil { | ||
| _ = f.Close() | ||
| _ = os.Remove(tmp) | ||
| return err | ||
| } | ||
|
|
||
| _ = resp.Body.Close() | ||
| if err := f.Close(); err != nil { | ||
| return fmt.Errorf("failed to close file: %w", err) | ||
| } |
There was a problem hiding this comment.
Ensure response bodies are closed on download errors.
If downloadWithProgress returns early, the response body is never closed, which can leak connections. Add a defer at the start of handleArtifactDownload and remove the manual close to cover all paths. (Line 1366.)
🔧 Proposed fix
func handleArtifactDownload(resp *http.Response, outDir, userFilename, name string) error {
+ defer func() { _ = resp.Body.Close() }()
contentType := resp.Header.Get("Content-Type")
serverFilename := extractServerFilename(resp)
compression := strings.TrimSpace(resp.Header.Get("X-AIB-Compression"))
filename := resolveFilename(userFilename, serverFilename, compression, name+".artifact")
@@
if err := downloadWithProgress(resp, f); err != nil {
_ = f.Close()
_ = os.Remove(tmp)
return err
}
- _ = resp.Body.Close()
if err := f.Close(); err != nil {
return fmt.Errorf("failed to close file: %w", err)
}📝 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.
| func handleArtifactDownload(resp *http.Response, outDir, userFilename, name string) error { | |
| contentType := resp.Header.Get("Content-Type") | |
| serverFilename := extractServerFilename(resp) | |
| compression := strings.TrimSpace(resp.Header.Get("X-AIB-Compression")) | |
| filename := resolveFilename(userFilename, serverFilename, compression, name+".artifact") | |
| printArtifactMetadata(resp) | |
| outPath := filepath.Join(outDir, filename) | |
| tmp := outPath + ".partial" | |
| f, err := os.Create(tmp) | |
| if err != nil { | |
| _ = resp.Body.Close() | |
| return err | |
| } | |
| if err := downloadWithProgress(resp, f); err != nil { | |
| _ = f.Close() | |
| _ = os.Remove(tmp) | |
| return err | |
| } | |
| _ = resp.Body.Close() | |
| if err := f.Close(); err != nil { | |
| return fmt.Errorf("failed to close file: %w", err) | |
| } | |
| func handleArtifactDownload(resp *http.Response, outDir, userFilename, name string) error { | |
| defer func() { _ = resp.Body.Close() }() | |
| contentType := resp.Header.Get("Content-Type") | |
| serverFilename := extractServerFilename(resp) | |
| compression := strings.TrimSpace(resp.Header.Get("X-AIB-Compression")) | |
| filename := resolveFilename(userFilename, serverFilename, compression, name+".artifact") | |
| printArtifactMetadata(resp) | |
| outPath := filepath.Join(outDir, filename) | |
| tmp := outPath + ".partial" | |
| f, err := os.Create(tmp) | |
| if err != nil { | |
| _ = resp.Body.Close() | |
| return err | |
| } | |
| if err := downloadWithProgress(resp, f); err != nil { | |
| _ = f.Close() | |
| _ = os.Remove(tmp) | |
| return err | |
| } | |
| if err := f.Close(); err != nil { | |
| return fmt.Errorf("failed to close file: %w", err) | |
| } |
🤖 Prompt for AI Agents
In `@cmd/caib/main.go` around lines 1350 - 1375, In handleArtifactDownload, ensure
resp.Body is closed on all return paths by adding defer resp.Body.Close()
immediately after entering the function (near the start of
handleArtifactDownload) and remove the manual closes of resp.Body (the two `_ =
resp.Body.Close()` calls); keep explicit f.Close() and os.Remove(tmp) handling
around downloadWithProgress and final file close, and avoid double-closing
resp.Body so connections aren’t leaked when downloadWithProgress returns early.
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 (4)
cmd/caib/main.go (3)
723-733: Potential data loss: destination file close error is logged but not returned.When writing to
dst, aClose()error can indicate that data wasn't fully flushed to disk. The current pattern logs a warning but returnsnil, potentially hiding a corrupted/incomplete file.🔧 Proposed fix
- 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() return fmt.Errorf("copy layer blob: %w", err) } - return nil + if err := dst.Close(); err != nil { + return fmt.Errorf("close destination file: %w", err) + } + return nil
1500-1510: Security: Tar extraction is vulnerable to path traversal (zip slip).The tar header name is joined directly with
destDirwithout validation. A malicious archive containing entries like../../../etc/passwdcould write files outside the intended directory.🔧 Proposed fix
for { hdr, err := tr.Next() if err == io.EOF { break } if err != nil { return err } - targetPath := filepath.Join(destDir, hdr.Name) + // Clean the path and ensure it doesn't escape destDir + cleanName := filepath.Clean(hdr.Name) + if strings.HasPrefix(cleanName, "..") || filepath.IsAbs(cleanName) { + return fmt.Errorf("invalid tar entry path: %s", hdr.Name) + } + targetPath := filepath.Join(destDir, cleanName) + // Double-check the resolved path is within destDir + if !strings.HasPrefix(targetPath, filepath.Clean(destDir)+string(os.PathSeparator)) { + return fmt.Errorf("tar entry escapes destination: %s", hdr.Name) + } switch hdr.Typeflag {
1521-1527: Security: Symlink target is not validated.Malicious archives can use symlinks pointing outside
destDir(e.g.,../../../etc). Subsequent tar entries could then write through the symlink to arbitrary locations.🔧 Proposed fix
case tar.TypeSymlink: if err := os.MkdirAll(filepath.Dir(targetPath), 0o755); err != nil { return err } + // Validate symlink target doesn't escape destDir + linkTarget := hdr.Linkname + if !filepath.IsAbs(linkTarget) { + linkTarget = filepath.Join(filepath.Dir(targetPath), linkTarget) + } + linkTarget = filepath.Clean(linkTarget) + if !strings.HasPrefix(linkTarget, filepath.Clean(destDir)+string(os.PathSeparator)) && linkTarget != filepath.Clean(destDir) { + return fmt.Errorf("symlink target escapes destination: %s -> %s", hdr.Name, hdr.Linkname) + } if err := os.Symlink(hdr.Linkname, targetPath); err != nil && !os.IsExist(err) { return err }internal/buildapi/client/client.go (1)
1-55: Guard against a nilhttp.Clientfrom options.If a caller passes
WithHTTPClient(nil), the httpClient field becomes nil and subsequent requests will panic onc.httpClient.Do(). Either validate inNew()after applying options, or ignore nil values to preserve the default client.
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 1283-1305: If fmt.Sscan fails to parse Content-Length in
downloadWithKnownSize, avoid creating a zero-sized progress bar; detect the
parse error and immediately fall back to the unknown-size download path by
calling the spinner-based helper (e.g. downloadWithUnknownSize(resp, destFile))
and return its error. Specifically, after the fmt.Sscan check inside
downloadWithKnownSize, return downloadWithUnknownSize(...) when parsing fails
instead of continuing with total==0, and keep the rest of downloadWithKnownSize
unchanged for the successful-parse branch.
♻️ Duplicate comments (3)
cmd/caib/main.go (1)
1349-1382: Ensure response body is closed on all error paths.If
downloadWithProgressreturns an error (line 1366),resp.Bodyis not closed before returning, which can leak connections. The manual close on line 1372 only executes on the success path.🔧 Proposed fix
func handleArtifactDownload(resp *http.Response, outDir, userFilename, name string) error { + defer func() { _ = resp.Body.Close() }() contentType := resp.Header.Get("Content-Type") serverFilename := extractServerFilename(resp) compression := strings.TrimSpace(resp.Header.Get("X-AIB-Compression")) filename := resolveFilename(userFilename, serverFilename, compression, name+".artifact") printArtifactMetadata(resp) outPath := filepath.Join(outDir, filename) tmp := outPath + ".partial" f, err := os.Create(tmp) if err != nil { - _ = resp.Body.Close() return err } if err := downloadWithProgress(resp, f); err != nil { _ = f.Close() _ = os.Remove(tmp) return err } - _ = resp.Body.Close() if err := f.Close(); err != nil {test/utils/utils.go (1)
26-26: Drop thestaticchecksuppression for the Ginkgo dot-import.Only
reviveis typically required for dot-imports, and this remains inconsistent with other test files.💡 Suggested fix
- . "github.com/onsi/ginkgo/v2" //nolint:revive,staticcheck + . "github.com/onsi/ginkgo/v2" //nolint:revive // Dot import is standard for Ginkgo#!/bin/bash rg -n '^\s*\.\s+"github.com/onsi/ginkgo/v2"' -C1 -g '*.go' rg -n 'nolint:.*staticcheck' -g '*.go'internal/buildapi/server.go (1)
1382-1394: Shell command injection risk remains unaddressed.The
artifactFileNameis interpolated into a shell command without validation. WhilevalidateInputexists, it's not applied here. The past review comment already flagged this issue.
🧹 Nitpick comments (13)
internal/controller/catalogimage/registry.go (1)
88-92: Consider using structured logging instead of stderr for close errors.The deferred
Close()error handling logs toos.Stderr, which bypasses the structured logging infrastructure used elsewhere in the codebase. In Kubernetes operators, using the controller's logger is preferred for consistency and observability.Additionally, this pattern is repeated three times—consider extracting a helper.
♻️ Suggested approach
// Helper function at package level func closeImageSource(src types.ImageSource, log logr.Logger) { if err := src.Close(); err != nil { log.Error(err, "failed to close image source") } }Then use it in each method where a logger is available. For methods on
DefaultRegistryClientthat lack a logger field, you could either:
- Add a logger field to
DefaultRegistryClient- Accept the current approach as a reasonable fallback for lint compliance
Also applies to: 118-122, 334-338
internal/controller/image/controller.go (1)
19-22: Phase constants improve maintainability, but usage is inconsistent.The introduction of
phaseAvailableandphaseUnavailableconstants is a good practice. However, the"Verifying"phase is still used as a string literal throughout the file (lines 51, 67, 113, 132), and""is used for the initial state check (line 49).Consider defining constants for all phases to maintain consistency and prevent potential typos.
♻️ Suggested fix
const ( phaseAvailable = "Available" phaseUnavailable = "Unavailable" + phaseVerifying = "Verifying" + phaseInitial = "" )Then update usages throughout the file to use these constants.
internal/controller/imagebuild/controller.go (3)
478-489: Consider removing unusedbuildConfigassignment.The
buildConfigvariable is fetched fromOperatorConfigbut then immediately suppressed with_ = buildConfig. The comment claims it's "used for PVC sizing if needed" but the actual PVC sizing logic at lines 1476-1480 fetchesOperatorConfigindependently rather than using this variable. This appears to be dead code.Either implement the intended use or remove lines 478-489 to avoid confusion.
498-499: Consider reusing existingnsNamevariable to avoid shadowing.
nsNameis already declared at line 467 with identical value. This redeclaration shadows the outer variable unnecessarily.♻️ Suggested fix
- nsName := types.NamespacedName{Name: imageBuild.Name, Namespace: imageBuild.Namespace} - if err := r.Get(ctx, nsName, fresh); err != nil { + if err := r.Get(ctx, nsName, fresh); err != nil {
980-981: Consider reusing existingnsNamevariable.
nsNameis already declared at line 920 and can be reused here sincelatestImageBuildwas fetched using the same Name/Namespace asimageBuild.♻️ Suggested fix
freshBuild := &automotivev1alpha1.ImageBuild{} - nsName := types.NamespacedName{Name: latestImageBuild.Name, Namespace: latestImageBuild.Namespace} if err := r.Get(ctx, nsName, freshBuild); err != nil {cmd/caib/main.go (1)
1337-1347: Content-Disposition parsing may extract incorrect filename.The current parsing doesn't handle multiple parameters in the header (e.g.,
attachment; filename="file.img"; size=123). It would return"file.img"; size=123instead of justfile.img.♻️ Proposed fix
func extractServerFilename(resp *http.Response) string { cd := resp.Header.Get("Content-Disposition") if cd == "" { return "" } if i := strings.Index(cd, "filename="); i >= 0 { - return strings.Trim(cd[i+9:], "\" ") + val := cd[i+9:] + // Handle quoted filename + if strings.HasPrefix(val, "\"") { + if end := strings.Index(val[1:], "\""); end >= 0 { + return val[1 : end+1] + } + } + // Handle unquoted filename (ends at semicolon or end of string) + if semi := strings.Index(val, ";"); semi >= 0 { + return strings.TrimSpace(val[:semi]) + } + return strings.TrimSpace(val) } return "" }cmd/caib/catalog/get.go (2)
80-84: Consider writing warnings to stderr instead of stdout.Warnings and diagnostic messages are conventionally written to stderr to avoid polluting stdout, which may be piped or parsed by downstream tools.
Suggested change
defer func() { if err := resp.Body.Close(); err != nil { - fmt.Printf("Warning: failed to close response body: %v\n", err) + fmt.Fprintf(os.Stderr, "Warning: failed to close response body: %v\n", err) } }()
103-114: Good addition of unmarshal error handling.The error checks for
json.Unmarshalare a valuable improvement. Note that errors fromjson.MarshalIndent(line 106) andyaml.Marshal(line 113) are still ignored with_. While these are unlikely to fail for validmap[string]interface{}data, you may want to handle them for completeness.cmd/caib/catalog/add.go (1)
62-69: Addresses past review, but consider usingcobra.CheckErr()for consistency.This fixes the previous issue of returning
nilon error, which could cause downstream nil dereferences. However,os.Exit(1)during command construction (which typically happens at init time) may not be the most idiomatic approach.
cobra.CheckErr()is designed for this pattern and provides consistent error handling with Cobra's conventions.♻️ Optional: Use cobra.CheckErr() for idiomatic Cobra error handling
- if err := cmd.MarkFlagRequired("architecture"); err != nil { - fmt.Fprintf(os.Stderr, "failed to mark required flag 'architecture': %v\n", err) - os.Exit(1) - } - if err := cmd.MarkFlagRequired("distro"); err != nil { - fmt.Fprintf(os.Stderr, "failed to mark required flag 'distro': %v\n", err) - os.Exit(1) - } + cobra.CheckErr(cmd.MarkFlagRequired("architecture")) + cobra.CheckErr(cmd.MarkFlagRequired("distro"))internal/buildapi/server.go (1)
987-1010: Consider using the structured logger for consistency.The warning messages use
log.Printfwhile the rest of the codebase usesa.log(structured logger). This creates inconsistency in log output.♻️ Suggested refactor
if err := setConfigMapOwnerRef(ctx, k8sClient, namespace, cfgName, imageBuild); err != nil { - log.Printf( - "WARNING: failed to set owner reference on ConfigMap %s: %v (cleanup may require manual intervention)", - cfgName, err, - ) + a.log.Error(err, "failed to set owner reference on ConfigMap (cleanup may require manual intervention)", + "configMap", cfgName) }Apply similarly to the other
log.Printfcalls at lines 995-999 and 1005-1009.internal/controller/operatorconfig/controller.go (3)
238-241: Consider documenting or removing the unused parameter.The
_ *automotivev1alpha1.OperatorConfigparameter is unused. If it's for API consistency or future use, consider adding a brief comment. Otherwise, it could be removed.
270-277: Use theannotationKeyvariable consistently.
annotationKeyis defined at line 272 but the literal string is used again at line 277 instead of the variable.♻️ Suggested fix
annotationKey := "serviceaccounts.openshift.io/oauth-redirectreference.buildapi" if sa.Annotations[annotationKey] == buildAPIAnnotation { return nil // Already set } - sa.Annotations["serviceaccounts.openshift.io/oauth-redirectreference.buildapi"] = buildAPIAnnotation + sa.Annotations[annotationKey] = buildAPIAnnotation
329-333: Unusedownerparameter.The
_ *automotivev1alpha1.OperatorConfigparameter is unused. This appears to be the same pattern asensureBuildAPIOAuthSecret. Consider documenting why this parameter exists or removing it if unnecessary.
| // downloadWithKnownSize downloads with a sized progress bar | ||
| func downloadWithKnownSize(resp *http.Response, destFile *os.File, contentLength string) error { | ||
| var total int64 | ||
| if _, err := fmt.Sscan(contentLength, &total); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Warning: failed to parse Content-Length: %v\n", err) | ||
| } | ||
| bar := progressbar.NewOptions64( | ||
| total, | ||
| progressbar.OptionSetDescription("Downloading"), | ||
| progressbar.OptionShowBytes(true), | ||
| progressbar.OptionSetWidth(15), | ||
| progressbar.OptionThrottle(65*time.Millisecond), | ||
| progressbar.OptionShowCount(), | ||
| progressbar.OptionClearOnFinish(), | ||
| ) | ||
| reader := io.TeeReader(resp.Body, bar) | ||
| if _, copyErr := io.Copy(destFile, reader); copyErr != nil { | ||
| return copyErr | ||
| } | ||
| _ = bar.Finish() | ||
| fmt.Println() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Falls through with zero total if Content-Length parsing fails.
If fmt.Sscan fails to parse the Content-Length header, total remains 0 and a progress bar is created with size 0, which may display incorrectly. Consider falling back to the unknown-size spinner.
🔧 Proposed fix
func downloadWithKnownSize(resp *http.Response, destFile *os.File, contentLength string) error {
var total int64
if _, err := fmt.Sscan(contentLength, &total); err != nil {
- fmt.Fprintf(os.Stderr, "Warning: failed to parse Content-Length: %v\n", err)
+ fmt.Fprintf(os.Stderr, "Warning: failed to parse Content-Length: %v, using spinner\n", err)
+ return downloadWithUnknownSize(resp, destFile)
}
+ if total <= 0 {
+ return downloadWithUnknownSize(resp, destFile)
+ }
bar := progressbar.NewOptions64(📝 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.
| // downloadWithKnownSize downloads with a sized progress bar | |
| func downloadWithKnownSize(resp *http.Response, destFile *os.File, contentLength string) error { | |
| var total int64 | |
| if _, err := fmt.Sscan(contentLength, &total); err != nil { | |
| fmt.Fprintf(os.Stderr, "Warning: failed to parse Content-Length: %v\n", err) | |
| } | |
| bar := progressbar.NewOptions64( | |
| total, | |
| progressbar.OptionSetDescription("Downloading"), | |
| progressbar.OptionShowBytes(true), | |
| progressbar.OptionSetWidth(15), | |
| progressbar.OptionThrottle(65*time.Millisecond), | |
| progressbar.OptionShowCount(), | |
| progressbar.OptionClearOnFinish(), | |
| ) | |
| reader := io.TeeReader(resp.Body, bar) | |
| if _, copyErr := io.Copy(destFile, reader); copyErr != nil { | |
| return copyErr | |
| } | |
| _ = bar.Finish() | |
| fmt.Println() | |
| return nil | |
| } | |
| // downloadWithKnownSize downloads with a sized progress bar | |
| func downloadWithKnownSize(resp *http.Response, destFile *os.File, contentLength string) error { | |
| var total int64 | |
| if _, err := fmt.Sscan(contentLength, &total); err != nil { | |
| fmt.Fprintf(os.Stderr, "Warning: failed to parse Content-Length: %v, using spinner\n", err) | |
| return downloadWithUnknownSize(resp, destFile) | |
| } | |
| if total <= 0 { | |
| return downloadWithUnknownSize(resp, destFile) | |
| } | |
| bar := progressbar.NewOptions64( | |
| total, | |
| progressbar.OptionSetDescription("Downloading"), | |
| progressbar.OptionShowBytes(true), | |
| progressbar.OptionSetWidth(15), | |
| progressbar.OptionThrottle(65*time.Millisecond), | |
| progressbar.OptionShowCount(), | |
| progressbar.OptionClearOnFinish(), | |
| ) | |
| reader := io.TeeReader(resp.Body, bar) | |
| if _, copyErr := io.Copy(destFile, reader); copyErr != nil { | |
| return copyErr | |
| } | |
| _ = bar.Finish() | |
| fmt.Println() | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@cmd/caib/main.go` around lines 1283 - 1305, If fmt.Sscan fails to parse
Content-Length in downloadWithKnownSize, avoid creating a zero-sized progress
bar; detect the parse error and immediately fall back to the unknown-size
download path by calling the spinner-based helper (e.g.
downloadWithUnknownSize(resp, destFile)) and return its error. Specifically,
after the fmt.Sscan check inside downloadWithKnownSize, return
downloadWithUnknownSize(...) when parsing fails instead of continuing with
total==0, and keep the rest of downloadWithKnownSize unchanged for the
successful-parse branch.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
CI/CD
✏️ Tip: You can customize this high-level summary in your review settings.