Improve API from kube-api-linter suggestions#329
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
* and other linter suggested fixes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughRefactors the MustGather API types from value to pointer-based fields, converting ServiceAccountName, ImageStreamRef.Name/Tag, Audit, and other optional fields to pointers with nil-safety checks. Updates reconciliation logic, templates, and tests to handle pointer dereferencing with appropriate defaults. Adds kubeapilinter configuration and updates dependency tracking. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Comment |
|
/test all |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: swghosh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #329 +/- ##
==========================================
+ Coverage 58.56% 58.93% +0.36%
==========================================
Files 8 8
Lines 806 823 +17
==========================================
+ Hits 472 485 +13
- Misses 322 326 +4
Partials 12 12
🚀 New features to boost your workflow:
|
swghosh
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@swghosh: This pull request references MG-91 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
swghosh
changed the title
openshift-ci-robot
removed
the
jira/valid-reference
|
@swghosh: No Jira issue is referenced in the title of this pull request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/mustgather/mustgather_controller.go`:
- Around line 431-435: The guard only rejects nil pointers but allows empty
strings; update the pre-check around instance.Spec.ImageStreamRef to also reject
empty values by verifying Name and Tag are non-nil and non-empty before
dereferencing (e.g., ensure *instance.Spec.ImageStreamRef.Name != "" and
*instance.Spec.ImageStreamRef.Tag != ""); if either is nil or empty return the
same fmt.Errorf("imageStreamRef name and tag must be specified") error, then
continue to assign isName/isTag after the validated check.
- Around line 238-241: The sftp host handling currently treats a
present-but-empty Host as a valid value and passes an empty string into
validateSFTPWithRetry(); change the normalization so Host is treated as unset
when nil OR empty (same behavior as ServiceAccountName) — e.g., in the code that
sets sftpHost (and mirror the logic used in getJobTemplate()) check both
instance.Spec.UploadTarget.SFTP.Host != nil AND
*instance.Spec.UploadTarget.SFTP.Host != "" before using it, otherwise fall back
to "sftp.access.redhat.com"; ensure validateSFTPWithRetry() and getJobTemplate()
see the same normalized sftpHost value.
In `@controllers/mustgather/template.go`:
- Around line 134-137: The code currently falls back to EmptyDir when
storage.PersistentVolume.Claim.Name is nil; instead, in the function that
constructs outputVolume (referencing variables outputVolume, storage,
storage.Type, v1alpha1.StorageTypePersistentVolume, and
storage.PersistentVolume.Claim.Name) detect a nil or empty Claim.Name and return
or propagate a validation error immediately (rejecting the request) rather than
creating an EmptyDir; validate both nil and empty-string cases and include a
clear error message like "persistent volume claim name required" so callers can
surface the problem.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37b2f3f3-f632-4ec5-8046-7efc4dda475b
📒 Files selected for processing (8)
.golangci.ymlapi/v1alpha1/mustgather_types.gocontrollers/mustgather/mustgather_controller.gocontrollers/mustgather/mustgather_controller_test.gocontrollers/mustgather/template.gocontrollers/mustgather/template_test.gocontrollers/mustgather/validation.gocontrollers/mustgather/validation_test.go
| sftpHost := "sftp.access.redhat.com" | ||
| if instance.Spec.UploadTarget.SFTP.Host != nil { | ||
| sftpHost = *instance.Spec.UploadTarget.SFTP.Host | ||
| } |
There was a problem hiding this comment.
Treat Host: "" as unset.
If Host is present but empty, this bypasses the default and feeds an empty address into validateSFTPWithRetry(). That makes the preflight check fail instead of using sftp.access.redhat.com. Use the same nil-or-empty handling as ServiceAccountName, and keep getJobTemplate() aligned so both code paths normalize the host identically.
♻️ Suggested fix
sftpHost := "sftp.access.redhat.com"
- if instance.Spec.UploadTarget.SFTP.Host != nil {
+ if instance.Spec.UploadTarget.SFTP.Host != nil && *instance.Spec.UploadTarget.SFTP.Host != "" {
sftpHost = *instance.Spec.UploadTarget.SFTP.Host
}📝 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.
| sftpHost := "sftp.access.redhat.com" | |
| if instance.Spec.UploadTarget.SFTP.Host != nil { | |
| sftpHost = *instance.Spec.UploadTarget.SFTP.Host | |
| } | |
| sftpHost := "sftp.access.redhat.com" | |
| if instance.Spec.UploadTarget.SFTP.Host != nil && *instance.Spec.UploadTarget.SFTP.Host != "" { | |
| sftpHost = *instance.Spec.UploadTarget.SFTP.Host | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controllers/mustgather/mustgather_controller.go` around lines 238 - 241, The
sftp host handling currently treats a present-but-empty Host as a valid value
and passes an empty string into validateSFTPWithRetry(); change the
normalization so Host is treated as unset when nil OR empty (same behavior as
ServiceAccountName) — e.g., in the code that sets sftpHost (and mirror the logic
used in getJobTemplate()) check both instance.Spec.UploadTarget.SFTP.Host != nil
AND *instance.Spec.UploadTarget.SFTP.Host != "" before using it, otherwise fall
back to "sftp.access.redhat.com"; ensure validateSFTPWithRetry() and
getJobTemplate() see the same normalized sftpHost value.
| if instance.Spec.ImageStreamRef.Name == nil || instance.Spec.ImageStreamRef.Tag == nil { | ||
| return "", fmt.Errorf("imageStreamRef name and tag must be specified") | ||
| } | ||
| isName := *instance.Spec.ImageStreamRef.Name | ||
| isTag := *instance.Spec.ImageStreamRef.Tag |
There was a problem hiding this comment.
Reject empty ImageStream ref values in the upfront guard.
This condition only rejects nil pointers. name: "" or tag: "" falls through to the later imagestream lookup/tag scan, so users get a low-level lookup error instead of the intended validation failure.
♻️ Suggested fix
- if instance.Spec.ImageStreamRef.Name == nil || instance.Spec.ImageStreamRef.Tag == nil {
+ if instance.Spec.ImageStreamRef.Name == nil || *instance.Spec.ImageStreamRef.Name == "" ||
+ instance.Spec.ImageStreamRef.Tag == nil || *instance.Spec.ImageStreamRef.Tag == "" {
return "", fmt.Errorf("imageStreamRef name and tag must be specified")
}📝 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.
| if instance.Spec.ImageStreamRef.Name == nil || instance.Spec.ImageStreamRef.Tag == nil { | |
| return "", fmt.Errorf("imageStreamRef name and tag must be specified") | |
| } | |
| isName := *instance.Spec.ImageStreamRef.Name | |
| isTag := *instance.Spec.ImageStreamRef.Tag | |
| if instance.Spec.ImageStreamRef.Name == nil || *instance.Spec.ImageStreamRef.Name == "" || | |
| instance.Spec.ImageStreamRef.Tag == nil || *instance.Spec.ImageStreamRef.Tag == "" { | |
| return "", fmt.Errorf("imageStreamRef name and tag must be specified") | |
| } | |
| isName := *instance.Spec.ImageStreamRef.Name | |
| isTag := *instance.Spec.ImageStreamRef.Tag |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controllers/mustgather/mustgather_controller.go` around lines 431 - 435, The
guard only rejects nil pointers but allows empty strings; update the pre-check
around instance.Spec.ImageStreamRef to also reject empty values by verifying
Name and Tag are non-nil and non-empty before dereferencing (e.g., ensure
*instance.Spec.ImageStreamRef.Name != "" and *instance.Spec.ImageStreamRef.Tag
!= ""); if either is nil or empty return the same fmt.Errorf("imageStreamRef
name and tag must be specified") error, then continue to assign isName/isTag
after the validated check.
| if storage != nil && storage.Type == v1alpha1.StorageTypePersistentVolume && storage.PersistentVolume.Claim.Name != nil { | ||
| outputVolume.VolumeSource = corev1.VolumeSource{ | ||
| PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ | ||
| ClaimName: storage.PersistentVolume.Claim.Name, | ||
| ClaimName: *storage.PersistentVolume.Claim.Name, |
There was a problem hiding this comment.
Fail fast when PersistentVolume storage is missing claim.name.
This branch leaves outputVolume as EmptyDir whenever Claim.Name is nil. That silently turns a persistent-storage request into ephemeral storage and can drop the collected bundle instead of surfacing a validation error. Please reject a missing or empty claim name here rather than downgrading the storage mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controllers/mustgather/template.go` around lines 134 - 137, The code
currently falls back to EmptyDir when storage.PersistentVolume.Claim.Name is
nil; instead, in the function that constructs outputVolume (referencing
variables outputVolume, storage, storage.Type,
v1alpha1.StorageTypePersistentVolume, and storage.PersistentVolume.Claim.Name)
detect a nil or empty Claim.Name and return or propagate a validation error
immediately (rejecting the request) rather than creating an EmptyDir; validate
both nil and empty-string cases and include a clear error message like
"persistent volume claim name required" so callers can surface the problem.
|
@swghosh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
needs-rebase
3 participants
Running the kube-api-linter on the APIs provided by this operator suggested issues,
which were attempted to be fixed by the /golang:lint-fix Claude plugin.
/cc @shivprakashmuley @siddhibhor-56
Details