CNF-26103: Add default case in istiocsr getIssuer to prevent nil-pointer#463
CNF-26103: Add default case in istiocsr getIssuer to prevent nil-pointer#463sebrandon1 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@sebrandon1: This pull request references CNF-26103 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 story to target the "5.0.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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesIssuer kind validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/istiocsr/deployments.go (1)
499-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a focused test for the new default branch.
The existing invalid-kind reconciliation test fails earlier in
assertIssuerRefExists(lines 238-240), so it does not executegetIssuer’s new error path. Add a directgetIssuertest asserting the returned error and nil object.🤖 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 `@pkg/controller/istiocsr/deployments.go` around lines 499 - 500, Add a focused unit test for getIssuer that supplies an unsupported issuerRefKind and verifies it returns a nil issuer object plus the expected “unsupported issuer kind” error, bypassing assertIssuerRefExists so the default branch is exercised directly.
🤖 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.
Nitpick comments:
In `@pkg/controller/istiocsr/deployments.go`:
- Around line 499-500: Add a focused unit test for getIssuer that supplies an
unsupported issuerRefKind and verifies it returns a nil issuer object plus the
expected “unsupported issuer kind” error, bypassing assertIssuerRefExists so the
default branch is exercised directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 707c2ef0-8710-4873-b407-40bd0a74bd2b
📒 Files selected for processing (1)
pkg/controller/istiocsr/deployments.go
The getIssuer switch statement only handled clusterissuer and issuer kinds, leaving the object variable nil for any other kind. Although callers currently validate the kind beforehand, a direct call to getIssuer with an unsupported kind would panic on the subsequent Get() call. Return a descriptive error instead.
864c7cf to
23b282d
Compare
|
@sebrandon1: The following test 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. |
Summary
defaultcase to thegetIssuerswitch statement that returns a descriptive error instead of leavingobjectas nilr.Get()callassertIssuerRefExistsvalidating the kind first, but the function itself was unsafe for direct callersTest plan
go test ./pkg/controller/istiocsr/... -count=1— 117 passed)make lintcleanSummary by CodeRabbit