CM-841: Fix golangci-lint errors #361
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@anandkuma77: This pull request references CM-841 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 "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. |
WalkthroughThis pull request enhances Go linting configuration, improves error handling with explicit wrapping and type assertions across the istiocsr controller, updates test mocks to use consistent error sentinels, refactors internal helper methods for type safety, and makes minor bug fixes and code cleanup throughout the operator codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ Finishing touches
Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/controller/deployment/credentials_request.go (1)
46-56: Consider evaluating the wrapcheck suppressions for consistency with error handling patterns.Lines 48 wraps the error with context (
fmt.Errorf), but lines 50 and 55 return errors directly with//nolint:wrapcheckcomments claiming "error from lister is already contextual."While Kubernetes lister/informer errors typically do include method context (e.g.,
secrets.Get(),infrastructure.Get()), the inconsistency between wrapping some errors and suppressing wrapping for others may warrant clarification: Is the distinction intentional (some errors need additional context, others don't), or should all be wrapped consistently for clarity?This doesn't block the PR, but ensure the pattern aligns with your team's error handling conventions.
pkg/controller/deployment/deployment_overrides.go (1)
196-201: Consider wrapping the error for consistency and better debugging.While the lister error includes the resource name and namespace, wrapping it with operation context (similar to line 198) would improve debugging by clarifying this occurred during trusted CA configmap loading.
🔎 Proposed refactor to wrap the error
_, err := configmapinformer.Lister().ConfigMaps(operatorclient.TargetNamespace).Get(trustedCAConfigmapName) if err != nil && apierrors.IsNotFound(err) { return fmt.Errorf("(Retrying) trusted CA config map %q doesn't exist due to %w", trustedCAConfigmapName, err) } else if err != nil { - return err //nolint:wrapcheck // error from lister is already contextual + return fmt.Errorf("failed to get trusted CA config map %q: %w", trustedCAConfigmapName, err) }pkg/controller/deployment/default_cert_manager_controller.go (1)
58-58: Consider wrapping the error instead of suppressing the linter.While the Kubernetes client error contains API-level details, wrapping it would add application context about the specific operation that failed (creating the default cert manager). This improves debugging by making error messages more informative in logs and traces.
🔎 Proposed refactor to wrap the error
- return c.certManagerClient.CertManagers().Create(ctx, cm, metav1.CreateOptions{}) //nolint:wrapcheck // error from client is already contextual + cm, err := c.certManagerClient.CertManagers().Create(ctx, cm, metav1.CreateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to create default cert manager: %w", err) + } + return cm, nilDon't forget to import
fmtif not already present.pkg/controller/istiocsr/utils.go (1)
404-423: Consider simplifying the redundant type assertions.In these generic functions, after the type switch on
desired, the types are already known. The additionalany(desired).(*rbacv1.ClusterRole)assertions are technically redundant since the switch case already confirms the type. However, this doesn't affect correctness and satisfies the linter.Also applies to: 425-444, 446-465
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (29)
.golangci.yamlapi/operator/v1alpha1/certmanager_types.goapi/operator/v1alpha1/istiocsr_types.gomain.gopkg/controller/deployment/credentials_request.gopkg/controller/deployment/default_cert_manager_controller.gopkg/controller/deployment/deployment_overrides.gopkg/controller/deployment/deployment_unsupported_overrides.gopkg/controller/deployment/generic_deployment_controller.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/client.gopkg/controller/istiocsr/controller.gopkg/controller/istiocsr/controller_test.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/deployments_test.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services_test.gopkg/controller/istiocsr/test_utils.gopkg/controller/istiocsr/utils.gopkg/features/features.gopkg/operator/operatorclient/operatorclient.gopkg/operator/optionalinformer/optional_informer.gopkg/operator/setup_manager.gopkg/operator/starter.go
💤 Files with no reviewable changes (1)
- pkg/controller/deployment/generic_deployment_controller.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/istiocsr/controller.gopkg/operator/setup_manager.gopkg/operator/starter.gopkg/controller/deployment/default_cert_manager_controller.gopkg/operator/optionalinformer/optional_informer.gopkg/controller/deployment/deployment_overrides.goapi/operator/v1alpha1/istiocsr_types.gopkg/features/features.gopkg/controller/istiocsr/controller_test.gopkg/controller/deployment/deployment_unsupported_overrides.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/operator/operatorclient/operatorclient.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/services_test.gopkg/controller/istiocsr/client.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/deployments_test.gopkg/controller/deployment/credentials_request.gopkg/controller/istiocsr/utils.gopkg/controller/istiocsr/certificates_test.goapi/operator/v1alpha1/certmanager_types.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/test_utils.gomain.go
🧬 Code graph analysis (4)
pkg/controller/istiocsr/rbacs.go (2)
pkg/controller/istiocsr/controller.go (1)
Reconciler(48-55)api/operator/v1alpha1/istiocsr_types.go (1)
IstioCSR(48-64)
pkg/controller/istiocsr/deployments.go (3)
pkg/controller/istiocsr/errors.go (1)
NewIrrecoverableError(28-37)api/operator/v1alpha1/istiocsr_types.go (1)
IstioCSRConfig(80-159)api/operator/v1alpha1/certmanager_types.go (1)
CertManager(191-200)
pkg/controller/istiocsr/utils.go (3)
api/operator/v1alpha1/groupversion_info.go (1)
SchemeGroupVersion(29-29)api/operator/v1alpha1/istiocsr_types.go (1)
IstioCSR(48-64)api/operator/v1alpha1/conditions.go (2)
Ready(27-27)ReasonFailed(31-31)
pkg/controller/istiocsr/networkpolicies.go (2)
api/operator/v1alpha1/certmanager_types.go (1)
NetworkPolicy(247-272)pkg/operator/applyconfigurations/operator/v1alpha1/networkpolicy.go (1)
NetworkPolicy(20-22)
🔇 Additional comments (42)
pkg/features/features.go (1)
22-27: Well-reasoned lint suppression with clear justification.The
//nolint:wrapcheckannotation is appropriate here. The comment clearly explains thatfeaturegate.Set()already returns contextually meaningful errors, eliminating the need for additional error wrapping. This is a pragmatic approach to lint fixes that avoids artificial wrapper layers when they don't add value.pkg/controller/deployment/credentials_request.go (1)
23-23: Lint suppressions for path/volume constants are appropriate.The
//nolint:goseccomments correctly identify these as path and volume name constants rather than hardcoded credentials. This aligns with the security intent behind the G101 rule.Also applies to: 29-29
pkg/controller/deployment/deployment_unsupported_overrides.go (2)
42-45: LGTM!The
wrapchecksuppression is appropriate here.json.Unmarshalerrors already include sufficient context about the unmarshaling failure, and the calling context (UnsupportedConfigOverrides.Raw) is evident from the surrounding code.
47-48: LGTM!The suppression is well-justified. Since
unsupportedConfigOverridesmodifies the deployment via pointer, the assignment is technically redundant but keeping it documents the intent that the deployment is being transformed at this point.pkg/controller/deployment/deployment_overrides.go (1)
35-35: LGTM! Appropriate gosec suppression.The nolint comment correctly suppresses the G101 (hardcoded credentials) warning. This is a standard Kubernetes service account token directory path, not a credential.
api/operator/v1alpha1/istiocsr_types.go (1)
46-48: LGTM!The
nolint:godotdirective with explanatory comment is appropriate for suppressing the linter on operator-sdk annotations that don't require trailing periods.api/operator/v1alpha1/certmanager_types.go (1)
189-191: LGTM!Consistent with the same pattern applied to
IstioCSRtype.pkg/operator/optionalinformer/optional_informer.go (1)
57-58: LGTM!The
nolint:wrapcheckis justified here since Kubernetes discovery client errors include sufficient context. The inline comment documents the rationale.pkg/operator/operatorclient/operatorclient.go (3)
172-176: Good rename fromcopytoupdated.Avoiding the builtin function name
copyimproves code clarity and prevents potential confusion.
207-214: LGTM!Using
slices.Containsis cleaner than manual iteration. The variable reuse forfinalizersis functionally equivalent to the previous approach.
249-253: LGTM!The
nolint:wrapcheckdirectives throughout this file are consistent with the configured exclusions for Kubernetes client interfaces where wrapping adds no value..golangci.yaml (3)
60-61: LGTM on depguard disable.Good practice to document the issue reference for the temporary disable.
224-250: Comprehensive wrapcheck configuration.The exclusions for internal packages and Kubernetes client interfaces are well-structured and reduce false positives without compromising error handling quality.
275-279: The staticcheck configuration format is correct for golangci-lint v2. The use ofchecks: ["all", "-SA4006", "-SA4031"]properly disables specific checks while enabling all others, and the explanatory comments justify the disables appropriately.pkg/controller/istiocsr/test_utils.go (1)
39-41: Good rename following Go conventions.Prefixing error variables with
err(e.g.,errTestClient) follows Go naming conventions and improves code consistency.main.go (1)
30-31: LGTM!Explicitly ignoring the
Help()error is acceptable here since the program exits immediately afterward regardless of success or failure.pkg/controller/istiocsr/serviceaccounts_test.go (2)
45-50: LGTM!Consistent usage of the renamed
errTestClientvariable from test_utils.go.
55-63: LGTM!Test case correctly uses the renamed error variable while maintaining the expected error message.
pkg/controller/istiocsr/networkpolicies.go (1)
47-50: LGTM! Type-safe assertion prevents panics.The checked type assertion correctly handles type mismatches by returning a descriptive error instead of panicking. This aligns with the PR's forcetypeassert lint fix objective.
pkg/operator/setup_manager.go (1)
82-82: LGTM! Appropriate lint suppression.The nolint directive is justified since manager errors already provide contextual information, making additional wrapping redundant.
pkg/controller/istiocsr/certificates.go (1)
77-81: LGTM! Efficient slice pre-allocation.Pre-allocating the slice capacity prevents reallocations during subsequent appends, improving performance while maintaining the same functional behavior.
pkg/controller/istiocsr/services_test.go (1)
48-48: LGTM! Consistent test error naming.The renaming from
testErrortoerrTestClientimproves clarity and aligns with the broader test refactoring across IstioCSR test files.Also applies to: 61-61, 84-84
pkg/controller/istiocsr/certificates_test.go (1)
37-37: LGTM! Consistent test error naming.These changes align with the test refactoring to use
errTestClientconsistently across IstioCSR test files.Also applies to: 67-67, 116-116
pkg/controller/istiocsr/deployments_test.go (1)
119-119: LGTM! Consistent test error naming.All instances updated to use
errTestClient, maintaining consistency with the broader test refactoring effort.Also applies to: 170-170, 220-220, 363-363, 530-530, 566-566
pkg/controller/istiocsr/controller_test.go (1)
147-147: LGTM! Consistent test error naming.The error variable renaming maintains consistency with the test refactoring across all IstioCSR controller tests.
Also applies to: 379-379, 401-401, 682-682, 731-731
pkg/controller/istiocsr/install_instiocsr_test.go (1)
75-75: LGTM! Consistent test error naming.These final updates complete the consistent error variable naming across all IstioCSR test files.
Also applies to: 88-88, 104-104
pkg/controller/istiocsr/controller.go (2)
106-106: LGTM!The
nolint:wrapcheckcomment is appropriate here sincecache.Newreturns errors that are already contextual and don't need additional wrapping in this thin wrapper function.
130-130: LGTM!The lint suppression comments are well-justified:
nestifsuppressions for complex but necessary control flow in label processing and deployment reconciliationwrapcheckfor builder chain that returns contextual errorsunparamfor error return kept for future implementation (as noted in the TODO)Also applies to: 198-199, 275-275, 332-334
pkg/controller/istiocsr/client.go (2)
78-82: LGTM - Good type-safe object instantiation.The reflection-based object creation with explicit type assertion prevents potential panics from unchecked type assertions and provides a clear error message if the assertion fails.
46-46: LGTM!The
nolint:wrapcheckcomments are appropriate for these thin wrapper methods - the underlying client errors are already contextual and wrapping would add redundant information.Also applies to: 52-52, 58-58, 64-64, 70-70, 92-92, 101-101, 107-107, 115-115
pkg/operator/starter.go (2)
48-48: LGTM!The
nolint:wrapcheckcomments are appropriate -NewForConfigerrors are already contextual from the Kubernetes client library.Also applies to: 53-53, 58-58, 83-83
149-149: LGTM!The
nolint:contextcheckis correctly applied and well-documented.SetupSignalHandlerintentionally creates a new context for OS signal handling rather than using the passed context, which is the expected pattern for graceful shutdown.pkg/controller/istiocsr/rbacs_test.go (1)
31-31: LGTM!Consistent renaming of test error variable to
errTestClientfollows Go conventions for error variable naming (errprefix) and aligns with the updated sentinel defined in test_utils.go.Also applies to: 47-47, 63-63, 76-76, 90-90, 105-105, 119-119
pkg/controller/istiocsr/deployments.go (3)
32-32: LGTM!Renaming
invalidIssuerRefConfigErrortoerrInvalidIssuerRefConfigfollows Go naming conventions for package-level error variables.
253-260: LGTM - Type-safe issuer handling.The type switch with explicit handling for
ClusterIssuerandIssuertypes, plus a default case that returns an irrecoverable error, eliminates the risk of panics from unchecked type assertions while providing clear error messages for unexpected types.
366-373: LGTM!Consistent application of the same type switch pattern in
handleIssuerBasedCAensures type safety throughout the issuer handling code paths.pkg/controller/istiocsr/rbacs.go (2)
135-146: LGTM!Renaming parameter from
newtodesiredavoids shadowing Go's builtinnewfunction and improves code clarity by using consistent terminology (desiredvsexisting/fetched) throughout the codebase.
225-236: LGTM!Consistent parameter renaming applied to
updateClusterRoleBindingNameInStatus.pkg/controller/istiocsr/utils.go (4)
66-66: LGTM!The
nolint:wrapcheckis appropriate - the retry loop already wraps errors with context before returning.
163-173: LGTM - Hardened decode helpers.Adding checked type assertions to decode functions provides clear panic messages if static assets contain unexpected types. Since these decode static manifests bundled with the operator, a type mismatch indicates a programming/build error that should fail fast.
Also applies to: 175-185, 187-197, 199-209, 211-221, 223-233, 235-245, 247-257
265-322: LGTM!The added type assertions in
hasObjectChangedprovide defense-in-depth, though the initialreflect.TypeOfcheck at line 260-262 should already catch type mismatches. The explicit assertions give clearer panic messages.
502-517: LGTM - Good helper extraction.
handleProcessingRejectioncleanly consolidates the status condition update and annotation handling, reducing duplication and improving maintainability.
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
PillaiManish
left a comment
PillaiManish
left a comment
There was a problem hiding this comment.
Looks good to me, just we need to check why the CI job is failing.
| deployment, ok := obj.(*appsv1.Deployment) | ||
| if !ok { | ||
| panic(fmt.Sprintf("decoded object is not a Deployment, got %T", obj)) | ||
| } | ||
| return deployment |
There was a problem hiding this comment.
What was the reason to modify this?
There was a problem hiding this comment.
We are typecasting here, so we are checking if the typecasting is successful or not. The ok bool has value true if the typecasting successful otherwise it has false value
There was a problem hiding this comment.
We are doing it at multiple places.
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
I think we should have proper reasoning and trackers for all the exclusions to be fixed in the future.
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/issuer_acme_dns01_test.go (1)
295-314: Add deployment rollout wait to GCP credentials setup for consistency.The
setupAmbientGCPCredentialsfunction (lines 309-313) patches the subscription with environment variables but lacks the corresponding deployment rollout synchronization step present insetupAmbientAWSCredentials(lines 250-256). Both functions perform identical subscription patches and should synchronize the cert-manager deployment rollout to prevent potential test flakiness:By("waiting for cert-manager deployment to rollout with new credentials") err = waitForDeploymentRollout(ctx, "cert-manager", "cert-manager", 2*time.Minute) Expect(err).NotTo(HaveOccurred(), "timeout waiting for cert-manager deployment rollout")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
test/e2e/issuer_acme_dns01_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/issuer_acme_dns01_test.go
🔇 Additional comments (1)
test/e2e/issuer_acme_dns01_test.go (1)
250-252: LGTM! Improves test reliability.The addition of a synchronous wait for the cert-manager deployment rollout after patching the subscription prevents race conditions and ensures the deployment has picked up the new credentials before proceeding. This follows the established pattern used elsewhere in the test suite (lines 772, 841, 1095) and uses a consistent 2-minute timeout.
|
/retest |
1 similar comment
|
/retest |
anandkuma77
force-pushed
the
anandkuma77/cm-841-lint-fix
branch
from
f1825dd to
c3b053f
Compare
Fix forcetypeassert, contextcheck, wastedassign, and makezero lint errors across istiocsr and deployment controller packages. Disable depguard due to golangci-lint v2 configuration issues.
anandkuma77
force-pushed
the
anandkuma77/cm-841-lint-fix
branch
from
c3b053f to
28de263
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/controller/deployment/deployment_overrides.go (1)
197-200: Consider wrapping for consistency.The
wrapchecksuppression is acceptable for this lint-fix PR. However, note the inconsistency: Line 198 wrapsNotFounderrors with application context, while Line 200 returns other lister errors unwrapped. For consistency, consider wrapping both cases with context about the trusted CA configmap validation.♻️ Optional: Consistent error wrapping
- return err //nolint:wrapcheck // error from lister is already contextual + return fmt.Errorf("failed to get trusted CA config map %q: %w", trustedCAConfigmapName, err)pkg/controller/deployment/credentials_request.go (1)
50-50: Inconsistent error handling approach.While the wrapcheck suppressions are acceptable for this lint-fix PR, note that Line 48 wraps NotFound errors with context, but other lister errors (lines 50, 55) are returned without additional context. For consistency and better debuggability, consider wrapping all errors to indicate which operation failed.
♻️ Optional: Consistent error wrapping
- return err //nolint:wrapcheck // error from lister is already contextual + return fmt.Errorf("failed to get cloud secret %q: %w", secretName, err)- return err //nolint:wrapcheck // error from lister is already contextual + return fmt.Errorf("failed to get infrastructure config: %w", err)Also applies to: 55-55
pkg/controller/deployment/deployment_unsupported_overrides.go (1)
42-45: Consider wrapping the unmarshal error for better debugging context.While
json.Unmarshalerrors are self-descriptive, adding context about which configuration field failed to unmarshal would significantly aid production debugging.🔧 Suggested improvement
err := json.Unmarshal(operatorSpec.UnsupportedConfigOverrides.Raw, cfg) if err != nil { - return err //nolint:wrapcheck // json.Unmarshal error is already clear + return fmt.Errorf("failed to unmarshal unsupported config overrides: %w", err) }Add
fmtto imports:import ( "encoding/json" "fmt" // ... rest of imports )pkg/controller/istiocsr/deployments.go (1)
366-373: Consider extracting duplicate type-switch logic.This type switch is identical to the one in
assertIssuerRefExists(lines 253-260). Consider extracting a helper function likegetIssuerConfig(obj client.Object) (certmanagerv1.IssuerConfig, error)to reduce duplication.♻️ Suggested helper function
func getIssuerConfig(obj client.Object) (certmanagerv1.IssuerConfig, error) { switch typedObj := obj.(type) { case *certmanagerv1.ClusterIssuer: return typedObj.Spec.IssuerConfig, nil case *certmanagerv1.Issuer: return typedObj.Spec.IssuerConfig, nil default: return certmanagerv1.IssuerConfig{}, fmt.Errorf("unexpected issuer type %T, expected ClusterIssuer or Issuer", obj) } }.golangci.yaml (1)
275-279: Remove the redundant staticcheck exclusions from linters-settings.The checks exclusions at lines 278-279 duplicate the issues-level text pattern exclusion already in place at lines 147-149, which covers
SA4006|SA4031. These rules are already excluded globally; remove the redundantstaticcheck.checksconfiguration to simplify the config. If targeted exclusions are preferred instead of global, add//nolint:staticcheckdirectives at specific locations and remove both this and the issues-level pattern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (29)
.golangci.yamlapi/operator/v1alpha1/certmanager_types.goapi/operator/v1alpha1/istiocsr_types.gomain.gopkg/controller/deployment/credentials_request.gopkg/controller/deployment/default_cert_manager_controller.gopkg/controller/deployment/deployment_overrides.gopkg/controller/deployment/deployment_unsupported_overrides.gopkg/controller/deployment/generic_deployment_controller.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/client.gopkg/controller/istiocsr/controller.gopkg/controller/istiocsr/controller_test.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/deployments_test.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services_test.gopkg/controller/istiocsr/test_utils.gopkg/controller/istiocsr/utils.gopkg/features/features.gopkg/operator/operatorclient/operatorclient.gopkg/operator/optionalinformer/optional_informer.gopkg/operator/setup_manager.gopkg/operator/starter.go
💤 Files with no reviewable changes (1)
- pkg/controller/deployment/generic_deployment_controller.go
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/controller/istiocsr/certificates.go
- pkg/controller/deployment/default_cert_manager_controller.go
- pkg/operator/setup_manager.go
- pkg/features/features.go
- api/operator/v1alpha1/certmanager_types.go
- pkg/controller/istiocsr/certificates_test.go
- pkg/controller/istiocsr/deployments_test.go
- pkg/operator/operatorclient/operatorclient.go
- pkg/controller/istiocsr/rbacs_test.go
- pkg/controller/istiocsr/install_instiocsr_test.go
- pkg/operator/optionalinformer/optional_informer.go
- pkg/operator/starter.go
- pkg/controller/istiocsr/rbacs.go
- main.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/deployment/credentials_request.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/deployment/deployment_unsupported_overrides.goapi/operator/v1alpha1/istiocsr_types.gopkg/controller/istiocsr/services_test.gopkg/controller/istiocsr/controller.gopkg/controller/istiocsr/controller_test.gopkg/controller/deployment/deployment_overrides.gopkg/controller/istiocsr/utils.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/client.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/test_utils.go
🧬 Code graph analysis (3)
pkg/controller/istiocsr/utils.go (2)
api/operator/v1alpha1/istiocsr_types.go (1)
IstioCSR(48-64)api/operator/v1alpha1/conditions.go (2)
Ready(27-27)ReasonFailed(31-31)
pkg/controller/istiocsr/client.go (2)
pkg/version/version.go (1)
Get(24-32)pkg/controller/istiocsr/controller.go (1)
New(110-122)
pkg/controller/istiocsr/deployments.go (2)
pkg/controller/istiocsr/errors.go (1)
NewIrrecoverableError(28-37)api/operator/v1alpha1/certmanager_types.go (1)
CertManager(191-200)
🔇 Additional comments (23)
pkg/controller/deployment/deployment_overrides.go (1)
35-35: LGTM! Appropriate lint suppression.The
gosecG101 suppression is correct—this is a path constant, not a credential. The justification comment is clear.pkg/controller/deployment/credentials_request.go (1)
23-23: LGTM! Appropriate gosec suppressions.The nolint directives correctly suppress false-positive credential warnings on path and volume name constants. The justification comments clearly explain these are not actual credentials.
Also applies to: 29-29
pkg/controller/deployment/deployment_unsupported_overrides.go (1)
47-47: Lint suppression is acceptable.The
wastedassignsuppression is justified here. While the assignment appears redundant sinceunsupportedConfigOverridesmodifies the deployment in place, keeping the assignment maintains consistency with the function's return contract.api/operator/v1alpha1/istiocsr_types.go (1)
46-47: LGTM!The
nolint:godotdirective appropriately suppresses the linter for the operator-sdk annotation, which is a machine-readable marker that doesn't follow sentence conventions.pkg/controller/istiocsr/deployments.go (2)
32-32: LGTM!Renaming to
errInvalidIssuerRefConfigfollows Go's idiomatic naming convention for unexported error variables.
253-260: LGTM!The type switch safely handles both
ClusterIssuerandIssuertypes with a proper default case for unexpected types, eliminating potential panics from unsafe type assertions..golangci.yaml (2)
60-61: LGTM!Disabling
depguardwith a clear comment referencing the GitHub issue provides good documentation for future maintainers.
224-250: LGTM!The
wrapcheckconfiguration is well-structured with appropriate ignore patterns for internal packages and standard Kubernetes client interfaces where additional wrapping would add noise without value.pkg/controller/istiocsr/client.go (2)
33-41: LGTM!Using the manager's client directly is the correct approach - it ensures the reconciler reads from the same cache that the controller's watches use, preventing cache mismatch issues.
78-82: LGTM - safe type assertion pattern.The reflection-based approach correctly creates a new instance matching the concrete type of
obj, then safely asserts it toclient.Objectwith anokcheck. This eliminates the potential panic from direct type assertions while preserving the original retry-with-fresh-fetch semantics.pkg/controller/istiocsr/utils.go (3)
168-172: LGTM!Safe type assertion with explicit error message on mismatch. Panicking here is appropriate since a type mismatch indicates a programming error (wrong asset being decoded), not a runtime condition.
265-271: Redundant but linter-compliant type checks.The
okchecks on lines 267-270 will always succeed because line 260 already verifies type equality viareflect.TypeOf. While redundant at runtime, these satisfy theforcetypeassertlinter requirement. The pattern is acceptable for this lint-fix PR.
502-517: LGTM - good helper extraction!The
handleProcessingRejectionmethod consolidates the status condition update and annotation logic, reducing duplication indisallowMultipleIstioCSRInstances. The error aggregation properly handles partial failures.pkg/controller/istiocsr/networkpolicies.go (1)
47-50: LGTM!Safe type assertion with descriptive error return. Unlike the
decode*functions inutils.gothat panic (since they're for known static assets), this function correctly returns an error since the asset path comes from runtime iteration.pkg/controller/istiocsr/test_utils.go (1)
40-40: LGTM!Renaming to
errTestClientfollows Go's idiomatic naming convention for error variables, maintaining consistency with similar changes elsewhere in the PR.pkg/controller/istiocsr/serviceaccounts_test.go (1)
45-45: LGTM! Consistent test error sentinel renaming.The update from
testErrortoerrTestClientin test mocks is consistent with the broader test suite standardization mentioned in the PR summary.Also applies to: 58-58
pkg/controller/istiocsr/services_test.go (1)
48-48: LGTM! Consistent test error sentinel renaming.The changes align with the test suite standardization pattern, replacing
testErrorwitherrTestClientacross service-related test mocks.Also applies to: 61-61, 84-84
pkg/controller/istiocsr/controller.go (5)
106-106: Verify that cache.New provides sufficient error context.The
//nolint:wrapcheckdirective assumescache.Newreturns contextual errors. Ensure that errors fromcache.Newinclude enough information to identify the failure point during cache initialization.
130-130: LGTM! Complexity acknowledged in lint suppression.The
//nolint:nestifdirective appropriately acknowledges the complexity in label processing logic. While this could be refactored in a future PR, the suppression is reasonable given this PR's focus on resolving linter errors.
198-198: Verify that builder.Complete() provides sufficient error context.The
//nolint:wrapcheckdirective assumes errors frombuilder.Complete()are already contextual. Ensure that controller setup failures provide clear diagnostic information to identify which watch or configuration step failed.
275-275: LGTM! Complexity acknowledged in lint suppression.The
//nolint:nestifdirective appropriately acknowledges the complexity in deployment reconciliation error handling. The nested logic differentiates between irrecoverable and recoverable errors, which justifies the complexity.
332-334: LGTM! Appropriate use of nolint for future implementation.The
//nolint:unparamdirective is justified here. The function signature is intentionally designed to support future cleanup logic (as indicated by the TODO on line 335), even though the current implementation returns static values.pkg/controller/istiocsr/controller_test.go (1)
147-147: LGTM! Comprehensive test error sentinel standardization.All mock error returns have been consistently updated from
testErrortoerrTestClientacross controller test scenarios, maintaining alignment with the test suite standardization pattern.Also applies to: 379-379, 401-401, 605-605, 682-682, 731-731
- Update generic_deployment_controller.go - Refactor istiocsr certificates, client, deployments, and utils - Simplify and optimize controller code
- Rename invalidIssuerRefConfigError to errInvalidIssuerRefConfig to conform to errname lint rule - Wrap all error returns in client.go methods with fmt.Errorf for proper error wrapping - Wrap RetryOnConflict error in utils.go status update function This fixes 1 errname and 10 wrapcheck lint errors as requested.
|
/retest |
This reverts commit 1115c3f.
- Add type assertion checks with ok pattern in client.go UpdateWithRetry - Fix type assertions in deployments.go assertIssuerRefExists and handleIssuerBasedCA - Fix all type assertions in utils.go decode functions and type switch cases - Ensure all type assertions are properly checked before use
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/controller/istiocsr/deployments.go (1)
375-385: Consider improving error message clarity.The safe type assertions are correctly implemented. However, the error message "failed to fetch issuer" on type conversion failure is slightly misleading since the issuer was successfully fetched but the type conversion failed. A more accurate message would be "failed to convert issuer to expected type" or similar.
Additionally,
getIssueris called in bothassertIssuerRefExists(line 247) andhandleIssuerBasedCA(line 367), which results in duplicate API calls. Consider storing the result fromassertIssuerRefExiststo avoid the redundant call.♻️ Suggested improvements
Option 1: Improve error message
clusterIssuer, ok := obj.(*certmanagerv1.ClusterIssuer) if !ok { - return FromClientError(fmt.Errorf("failed to convert to ClusterIssuer"), "failed to fetch issuer") + return FromClientError(fmt.Errorf("failed to convert to ClusterIssuer"), "failed to convert issuer to expected type") }issuer, ok := obj.(*certmanagerv1.Issuer) if !ok { - return FromClientError(fmt.Errorf("failed to convert to Issuer"), "failed to fetch issuer") + return FromClientError(fmt.Errorf("failed to convert to Issuer"), "failed to convert issuer to expected type") }Option 2: Cache issuer object to avoid duplicate getIssuer calls
Consider modifying
assertIssuerRefExiststo return the fetched issuer object, and then pass it tohandleIssuerBasedCAinstead of fetching again.pkg/controller/istiocsr/utils.go (1)
260-357: LGTM! Consider reducing redundancy in type checks.The safe type assertions are correctly implemented and add defensive programming. However, since line 260 already verifies that
desiredandfetchedhave the same type usingreflect.TypeOf, the type assertions within each case block will always succeed. This redundancy doesn't cause issues but could be simplified.♻️ Optional simplification
Given the type equality check at line 260, you could simplify by either:
Option 1: Keep the initial type check and use direct type assertions (without ok-checks) in each case, relying on the upfront validation.
Option 2: Remove the initial type check and keep the ok-checks in each case (current defensive approach).
The current implementation prioritizes safety over brevity, which is a valid choice for production code.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
pkg/controller/istiocsr/client.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/istiocsr/client.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/istiocsr/utils.gopkg/controller/istiocsr/deployments.go
🧬 Code graph analysis (1)
pkg/controller/istiocsr/deployments.go (2)
pkg/controller/istiocsr/errors.go (2)
NewIrrecoverableError(28-37)FromClientError(61-71)test/e2e/config_template.go (1)
IssuerConfig(14-17)
🔇 Additional comments (3)
pkg/controller/istiocsr/deployments.go (1)
255-265: LGTM! Safe type assertions correctly implemented.The defensive type assertions with ok-checks are properly implemented, and the error handling using
NewIrrecoverableErroris appropriate for invalid issuer configurations. This change successfully addresses the golangci-lint forcetypeassert warnings.pkg/controller/istiocsr/utils.go (2)
168-256: LGTM! Defensive type assertions correctly implemented in decode functions.All decode functions consistently implement safe type assertions with ok-checks and panic on failure. The panic approach is appropriate here since these functions decode static assets, and type assertion failures would indicate programming errors (incorrect asset content) rather than runtime conditions.
443-515: LGTM! Generic functions correctly implement safe type assertions.The safe type assertions in the generic RBAC comparison functions are correctly implemented using the
any()conversion pattern required for Go generics. The panic on type assertion failure is appropriate since these would indicate type constraint violations (programming errors).
|
/retest |
…ntroller.go" This reverts commit 9f67d84.
Wrapped all unwrapped errors with fmt.Errorf using %w verb to properly preserve error context.
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 (1)
pkg/controller/istiocsr/client.go (1)
91-114: UpdateWithRetry: improve safety with DeepCopyObject and defensive error handling.
reflect.TypeOf(obj).Elem()will panic ifobjis nil or not a pointer. While all current callers pass valid objects, usingobj.DeepCopyObject()is safer and more idiomatic.- Wrapping the
Update(...)error infmt.Errorfis safe with k8s v0.33.2+ (errors.IsConflict properly unwraps), but explicitly returning unwrapped conflict errors improves clarity and follows defensive patterns in similar retry loops.Proposed change
func (c *ctrlClientImpl) UpdateWithRetry( ctx context.Context, obj client.Object, opts ...client.UpdateOption, ) error { key := client.ObjectKeyFromObject(obj) if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - currentInterface := reflect.New(reflect.TypeOf(obj).Elem()).Interface() - current, ok := currentInterface.(client.Object) + currentRuntime := obj.DeepCopyObject() + current, ok := currentRuntime.(client.Object) if !ok { - return fmt.Errorf("failed to convert to client.Object") + return fmt.Errorf("failed to convert to client.Object (got %T)", currentRuntime) } if err := c.Client.Get(ctx, key, current); err != nil { return fmt.Errorf("failed to fetch latest %q for update: %w", key, err) } obj.SetResourceVersion(current.GetResourceVersion()) if err := c.Client.Update(ctx, obj, opts...); err != nil { - return fmt.Errorf("failed to update %q resource: %w", key, err) + if apierrors.IsConflict(err) { + return err + } + return fmt.Errorf("failed to update %q resource: %w", key, err) } return nil }); err != nil { return fmt.Errorf("failed to update with retry %q: %w", key, err) } return nil }
🤖 Fix all issues with AI agents
In @pkg/controller/istiocsr/utils.go:
- Around line 593-610: The logic is inverted: when ignoreProcessing is false we
should allow the oldest instance to proceed (return nil) and when
ignoreProcessing is true we should mark/reject this instance; modify the branch
around ignoreProcessing so that if !ignoreProcessing the function returns nil,
otherwise perform the existing rejection flow (call
istiocsr.Status.SetCondition(... Ready false ...), call
r.updateCondition(istiocsr, nil) as needed, call
addProcessingRejectedAnnotation(istiocsr) and r.UpdateWithRetry(...), aggregate
condUpdateErr and annUpdateErr and return NewMultipleInstanceError(...) on
error); keep use of symbols istiocsr, ignoreProcessing, updateCondition,
addProcessingRejectedAnnotation, UpdateWithRetry and NewMultipleInstanceError to
locate and apply the change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/controller/istiocsr/client.gopkg/controller/istiocsr/utils.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/istiocsr/utils.gopkg/controller/istiocsr/client.go
🧬 Code graph analysis (1)
pkg/controller/istiocsr/utils.go (2)
pkg/controller/istiocsr/errors.go (1)
NewMultipleInstanceError(39-48)api/operator/v1alpha1/conditions.go (1)
Ready(27-27)
🔇 Additional comments (5)
pkg/controller/istiocsr/utils.go (4)
66-66: LGTM: Error wrapping improves debugging context.The error wrapping with
fmt.Errorfand%wprovides better context when status updates fail.
163-257: LGTM: Defensive type assertions in decode functions.The explicit type assertions with panic on failure are appropriate for decode functions that process static manifests. If the assertion fails, it indicates a programming error or corrupted manifest, justifying the panic.
259-362: Type assertions are redundant but defensively correct.The explicit type assertions in each case branch are technically redundant given the type equality check at line 260. However, they provide defense-in-depth against future code changes and satisfy the linter's requirements.
440-519: LGTM: Type assertions required in generic functions.The explicit type assertions within generic functions are necessary because Go's type system doesn't automatically narrow types in switch branches when using type parameters. The panic-on-failure approach is appropriate as the type switch should guarantee the type.
pkg/controller/istiocsr/client.go (1)
43-50: Good improvement: consistent error wrapping with operation + object key context.This should make debugging significantly easier while preserving the underlying error via
%w(including NotFound handling inExists).To ensure no behavior changes in callers that rely on Kubernetes sentinel checks, please verify
k8s.io/apimachinery/pkg/api/errors.IsNotFound/IsConflictunwrap wrapped errors for the dependency versions in this repo.Also applies to: 52-59, 61-69, 71-79, 81-89, 116-124, 126-134, 136-144
…ate tests - Fixed inverted condition: oldest instance now correctly returns nil to allow processing - Updated test expectations to match new error message format after wrapping errors with fmt.Errorf
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/controller/istiocsr/utils.go:
- Around line 593-612: Add a unit test for disallowMultipleIstioCSRInstances
that covers the allowed-oldest-instance branch: create two IstioCSR objects with
different creation timestamps, ensure the one with the earliest timestamp is
processed by calling disallowMultipleIstioCSRInstances (or invoking the
controller logic that uses it) and assert it returns nil (no error), does not
set the rejection annotation, and leaves the Ready condition unchanged/true;
similarly assert the newer instance still follows the existing rejection path.
Use the same helper setup/mocks as existing tests for the rejection branch and
reference the function disallowMultipleIstioCSRInstances, the Ready condition
(v1alpha1.Ready), and addProcessingRejectedAnnotation behavior to locate where
to assert outcomes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
pkg/controller/istiocsr/controller_test.gopkg/controller/istiocsr/deployments_test.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/utils.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/istiocsr/deployments_test.go
- pkg/controller/istiocsr/controller_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/istiocsr/utils.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts_test.go
🧬 Code graph analysis (2)
pkg/controller/istiocsr/utils.go (4)
api/operator/v1alpha1/groupversion_info.go (1)
SchemeGroupVersion(29-29)api/operator/v1alpha1/certmanager_types.go (1)
NetworkPolicy(247-272)pkg/operator/applyconfigurations/operator/v1alpha1/networkpolicy.go (1)
NetworkPolicy(20-22)api/operator/v1alpha1/conditions.go (2)
Ready(27-27)ReasonFailed(31-31)
pkg/controller/istiocsr/rbacs_test.go (1)
api/operator/v1alpha1/istiocsr_types.go (1)
IstioCSR(48-64)
🔇 Additional comments (5)
pkg/controller/istiocsr/serviceaccounts_test.go (1)
45-45: LGTM! Test updates align with error constant rename.The changes consistently replace
testErrorwitherrTestClientand update the expected error message to reflect the new error wrapping format introduced in the status update path. These updates maintain test accuracy with the refactored error handling.Also applies to: 58-58, 63-63
pkg/controller/istiocsr/rbacs_test.go (1)
31-31: LGTM! Test mocks consistently updated with renamed error constant.All test mock configurations have been updated to use
errTestClientinstead oftestError, maintaining consistency with the test utility refactoring. The error message expectations (lines 258, 296) correctly reflect the new error wrapping format with the "status for" phrase in the status update path.Also applies to: 47-47, 63-63, 76-76, 90-90, 105-105, 119-119, 176-176, 217-217, 250-250, 258-258, 287-287, 296-296, 313-313, 336-336, 358-358, 378-378, 403-403, 427-427, 450-450, 470-470, 495-495, 519-519
pkg/controller/istiocsr/utils.go (3)
66-66: LGTM! Enhanced error context for status updates.The additional error wrapping provides clearer context by including the resource identifier when status updates fail after retries, improving debuggability while maintaining the error chain.
168-172: LGTM! Safe type assertions address linter warnings.The explicit type assertions with descriptive panic messages appropriately address the golangci-lint
forcetypeassertwarnings. Since these decode functions are internal utilities called with known byte content corresponding to specific resource types, the panics serve as programming error detection rather than runtime error handling. The consistent pattern across all decode functions ensures maintainability.Also applies to: 180-184, 192-196, 204-208, 216-220, 228-232, 240-244, 252-256
267-275: LGTM! Type assertions ensure type safety in comparisons and generic functions.The explicit type assertions in
hasObjectChangedand the generic RBAC helper functions (rbacRoleRulesModified,rbacRoleBindingRefModified,rbacRoleBindingSubjectsModified) address theforcetypeassertlinter warnings while maintaining type safety.For the generic functions, the type assertions within switch cases are necessary because Go's type system doesn't automatically narrow generic type parameters. The panics provide clear diagnostics for programming errors while keeping the runtime behavior predictable for correctly typed inputs.
Also applies to: 277-285, 287-296, 298-306, 308-316, 318-327, 329-337, 339-347, 349-357, 443-461, 470-488, 497-515
Renamed invalidIssuerRefConfigError to errInvalidIssuerRefConfig to conform to errXxx format required by errname linter.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/controller/istiocsr/deployments.go:
- Line 239: Update the error message passed to NewIrrecoverableError (in the
errInvalidIssuerRefConfig creation for
spec.istioCSRConfig.certManager.issuerRef.kind) to correct the typo "anyof" to
"any of" so the message reads "can be any of `%s` or `%s`, configured: %s".
🧹 Nitpick comments (2)
pkg/controller/istiocsr/deployments.go (2)
375-385: Consider aligning error handling withassertIssuerRefExistsfor consistency.The defensive type assertions are correct, but the error handling differs from the similar code in
assertIssuerRefExists:
- Here:
FromClientError(fmt.Errorf("failed to convert to ClusterIssuer"), "failed to fetch issuer")- In
assertIssuerRefExists:NewIrrecoverableError(errInvalidIssuerRefConfig, "failed to convert to ClusterIssuer")Type conversion failures indicate programming errors (bugs in
getIssuer), not transient client errors. Consider usingNewIrrecoverableErrorwitherrInvalidIssuerRefConfigfor consistency, as the error semantics are identical in both functions.🔄 Proposed consistency fix
case clusterIssuerKind: clusterIssuer, ok := obj.(*certmanagerv1.ClusterIssuer) if !ok { - return FromClientError(fmt.Errorf("failed to convert to ClusterIssuer"), "failed to fetch issuer") + return NewIrrecoverableError(errInvalidIssuerRefConfig, "failed to convert to ClusterIssuer") } issuerConfig = clusterIssuer.Spec.IssuerConfig case issuerKind: issuer, ok := obj.(*certmanagerv1.Issuer) if !ok { - return FromClientError(fmt.Errorf("failed to convert to Issuer"), "failed to fetch issuer") + return NewIrrecoverableError(errInvalidIssuerRefConfig, "failed to convert to Issuer") } issuerConfig = issuer.Spec.IssuerConfig
255-265: Consider extracting a helper function to reduce duplication.Both
assertIssuerRefExists(lines 255-265) andhandleIssuerBasedCA(lines 375-385) contain nearly identical type assertion logic for converting the issuer object and extractingIssuerConfig. Consider extracting this into a helper function to follow the DRY principle.💡 Example helper function
// getIssuerConfig extracts IssuerConfig from a fetched issuer object based on kind. func getIssuerConfig(obj client.Object, issuerRefKind string) (certmanagerv1.IssuerConfig, error) { switch issuerRefKind { case clusterIssuerKind: clusterIssuer, ok := obj.(*certmanagerv1.ClusterIssuer) if !ok { return certmanagerv1.IssuerConfig{}, fmt.Errorf("failed to convert to ClusterIssuer") } return clusterIssuer.Spec.IssuerConfig, nil case issuerKind: issuer, ok := obj.(*certmanagerv1.Issuer) if !ok { return certmanagerv1.IssuerConfig{}, fmt.Errorf("failed to convert to Issuer") } return issuer.Spec.IssuerConfig, nil default: return certmanagerv1.IssuerConfig{}, fmt.Errorf("unsupported issuer kind: %s", issuerRefKind) } }Then both functions can use:
issuerConfig, err := getIssuerConfig(obj, issuerRefKind) if err != nil { return NewIrrecoverableError(errInvalidIssuerRefConfig, err.Error()) }Also applies to: 375-385
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/controller/istiocsr/deployments.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/istiocsr/deployments.go
🔇 Additional comments (2)
pkg/controller/istiocsr/deployments.go (2)
32-32: LGTM!The error variable renaming from
invalidIssuerRefConfigErrortoerrInvalidIssuerRefConfigcorrectly follows Go naming conventions for error variables and satisfies the errname linter requirement.
255-265: LGTM!The defensive type assertions with ok-checks correctly address the forcetypeassert linter requirement. While these assertions should always succeed given the
getIssuerimplementation, the defensive checks are valuable for catching potential bugs and make the code more robust.
|
/retest |
- Fixed makezero errors in certificates.go by initializing slice with length 0 and appropriate capacity - Fixed typo in deployments.go: changed 'anyof' to 'any of' in error message
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controller/istiocsr/deployments.go (2)
236-272: Fix incorrect “configured” value in issuerRef kind error message (currently always printsissuerKind).At Line 239 the last format arg is
issuerKind, so the message won’t reflect what the user actually configured.Proposed fix
func (r *Reconciler) assertIssuerRefExists(istiocsr *v1alpha1.IstioCSR) error { issuerRefKind := strings.ToLower(istiocsr.Spec.IstioCSRConfig.CertManager.IssuerRef.Kind) if issuerRefKind != clusterIssuerKind && issuerRefKind != issuerKind { - return NewIrrecoverableError(errInvalidIssuerRefConfig, "spec.istioCSRConfig.certManager.issuerRef.kind can be any of `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerKind) + return NewIrrecoverableError(errInvalidIssuerRefConfig, "spec.istioCSRConfig.certManager.issuerRef.kind can be any of `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerRefKind) }Also: the ok-checked assertions are a solid improvement and align with the PR lint objectives.
362-386: Don’t wrap type-conversion failures as “failed to fetch issuer” client errors.If the conversion fails, the issuer was fetched; it’s an unexpected object type / internal inconsistency. Using
FromClientError(..., "failed to fetch issuer")is misleading (and may affect retry behavior depending onFromClientErrorsemantics).Suggested adjustment
switch issuerRefKind { case clusterIssuerKind: clusterIssuer, ok := obj.(*certmanagerv1.ClusterIssuer) if !ok { - return FromClientError(fmt.Errorf("failed to convert to ClusterIssuer"), "failed to fetch issuer") + return NewIrrecoverableError(errInvalidIssuerRefConfig, "unexpected issuer object type for kind %q: got %T, want *certmanagerv1.ClusterIssuer", issuerRefKind, obj) } issuerConfig = clusterIssuer.Spec.IssuerConfig case issuerKind: issuer, ok := obj.(*certmanagerv1.Issuer) if !ok { - return FromClientError(fmt.Errorf("failed to convert to Issuer"), "failed to fetch issuer") + return NewIrrecoverableError(errInvalidIssuerRefConfig, "unexpected issuer object type for kind %q: got %T, want *certmanagerv1.Issuer", issuerRefKind, obj) } issuerConfig = issuer.Spec.IssuerConfig }
🧹 Nitpick comments (1)
pkg/controller/istiocsr/deployments.go (1)
32-32: Prefererrors.Newfor a non-formatted sentinel error.
fmt.Errorf("...")works here (no formatting), buterrors.New(...)is the idiomatic sentinel and makes it clearer this should never wrap another error.Proposed tweak
+import ( "crypto/x509" "encoding/pem" + "errors" "fmt" "os" "reflect" "strings" "unsafe" @@ -var errInvalidIssuerRefConfig = fmt.Errorf("invalid issuerRef config") +var errInvalidIssuerRefConfig = errors.New("invalid issuerRef config")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/deployments.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/deployments.go
🧬 Code graph analysis (1)
pkg/controller/istiocsr/certificates.go (1)
api/operator/v1alpha1/istiocsr_types.go (2)
IstioCSRConfig(80-159)IstiodTLSConfig(185-250)
🔇 Additional comments (1)
pkg/controller/istiocsr/certificates.go (1)
77-78: LGTM! Efficient pre-allocation strategy.The change correctly addresses the makezero lint error by initializing the slice with zero length and pre-allocating capacity for both
CertificateDNSNamesandRevisions. This prevents multiple reallocations during the append operations in the revision loop (lines 82-93), improving performance compared to the previous approach.
Fixed the test expectation for invalid issuerRef kind error message. The test expected 'anyof' but the actual error message uses 'any of'. Updated the test to match the actual error message format.
Fixed gocritic typeSwitchVar error by using type switch with assignment. Removed unnecessary explicit type arguments from generic function calls since Go can infer them from the function arguments. - Changed switch desired.(type) to switch desiredObj := desired.(type) - Removed explicit type arguments from rbacRoleRulesModified calls - Removed explicit type arguments from rbacRoleBindingRefModified calls - Removed explicit type arguments from rbacRoleBindingSubjectsModified calls
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/istiocsr/utils.go (1)
520-577: Fix namespace-scoped logic and error wrapping consistencyThe loop at line 544 skips only same-namespace items, which allows multiple IstioCSRs in the same namespace to both attempt processing (as long as they're not older than instances in other namespaces). This contradicts the statusMessage implying one instance total. Change the skip condition to
item.GetNamespace() == istiocsr.GetNamespace() && item.GetName() == istiocsr.GetName()to skip only self.Also, lines 572-574 return a plain
utilerrors.NewAggregate()on update failures, whereas lines 530 and 576 wrap errors inNewMultipleInstanceError(). This inconsistency causes the caller'sIsMultipleInstanceError(err)check to fail when updates fail, changing reconciliation behavior from returning nil to propagating an error. Always wrap the aggregated errors inNewMultipleInstanceError(), whether updates succeed or fail.Proposed fix
ignoreProcessing := false for _, item := range istiocsrList.Items { - if item.GetNamespace() == istiocsr.Namespace { + if item.GetNamespace() == istiocsr.GetNamespace() && item.GetName() == istiocsr.GetName() { continue } if item.CreationTimestamp.Time.Before(istiocsr.CreationTimestamp.Time) || // Even when timestamps are equal will skip processing. And if this ends // up in ignoring all istiocsr instances, which means user must have created // all in parallel, onus is on user to delete all and recreate just one required // instance of istiocsr. item.CreationTimestamp.Time.Equal(istiocsr.CreationTimestamp.Time) { ignoreProcessing = true } } if !ignoreProcessing { // This is the oldest instance, allow it to proceed return nil } // This instance should be rejected as there's an older or equally old instance var condUpdateErr, annUpdateErr error if istiocsr.Status.SetCondition(v1alpha1.Ready, metav1.ConditionFalse, v1alpha1.ReasonFailed, statusMessage) { condUpdateErr = r.updateCondition(istiocsr, nil) } if addProcessingRejectedAnnotation(istiocsr) { if err := r.UpdateWithRetry(r.ctx, istiocsr); err != nil { annUpdateErr = fmt.Errorf("failed to update reject processing annotation to %s/%s: %w", istiocsr.GetNamespace(), istiocsr.GetName(), err) } } - if condUpdateErr != nil || annUpdateErr != nil { - return utilerrors.NewAggregate([]error{condUpdateErr, annUpdateErr}) - } - - return NewMultipleInstanceError(fmt.Errorf("%s", statusMessage)) + agg := utilerrors.NewAggregate([]error{fmt.Errorf("%s", statusMessage), condUpdateErr, annUpdateErr}) + return NewMultipleInstanceError(agg) }
🧹 Nitpick comments (3)
pkg/controller/istiocsr/utils.go (3)
404-483: RBAC generic helpers: runtime assertions are redundant; if kept, improve panic contextBecause the type parameter forces
desiredandfetchedto the same concrete type, the extraany(desired).(*rbacv1.Role)assertions shouldn’t fail unless something is very off. If you keep these for linting, consider panicking with%Tcontext (like the decode helpers) to avoid opaque crashes.
163-257: Improve panic diagnostics in decode*ObjBytes helpersThe panic messages lose the actual decoded type, making troubleshooting harder. While panic is acceptable here since these helpers load only static embedded manifests (via
assets.MustAsset), improve diagnostics to aid debugging:Proposed improvement
func decodeDeploymentObjBytes(objBytes []byte) *appsv1.Deployment { obj, err := runtime.Decode(codecs.UniversalDecoder(appsv1.SchemeGroupVersion), objBytes) if err != nil { - panic(err) + panic(fmt.Errorf("failed to decode Deployment: %w", err)) } deployment, ok := obj.(*appsv1.Deployment) if !ok { - panic("failed to convert to *appsv1.Deployment") + panic(fmt.Sprintf("failed to convert decoded object (%T) to *appsv1.Deployment", obj)) } return deployment }Apply the same pattern to all other decode helpers.
259-326: hasObjectChanged: panics on unhandled types are safe given controlled call sites, but consider error return for future-proofingAll 12 call sites in the istiocsr package construct and pass explicitly typed objects (ClusterRole, Deployment, ConfigMap, Certificate, Service, NetworkPolicy, Role, and RoleBinding variants). While the function accepts the
client.Objectinterface and panics in the default case, this is only reachable if a new object type is added to the callers without updating the switch statement—indicating a development-time error rather than a runtime crash risk.That said, returning
(bool, error)instead of panicking would improve robustness and allow callers to handle unsupported types gracefully, especially if this function is ever made public or used beyond the current controlled set of callers.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/controller/istiocsr/deployments_test.gopkg/controller/istiocsr/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/istiocsr/deployments_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/controller/istiocsr/utils.go
🧬 Code graph analysis (1)
pkg/controller/istiocsr/utils.go (2)
api/operator/v1alpha1/groupversion_info.go (1)
SchemeGroupVersion(29-29)api/operator/v1alpha1/conditions.go (2)
Ready(27-27)ReasonFailed(31-31)
🔇 Additional comments (1)
pkg/controller/istiocsr/utils.go (1)
49-70: Good: status update now returns contextual, wrapped errorsWrapping the RetryOnConflict failure with
failed to update status for ...(Line 66) makes reconcile failures easier to diagnose.
…nt controller The informers variable was being built but never used (nil was passed instead), causing a wastedassign lint error. However, removing the variable caused E2E test failures because the controller needs to watch ConfigMaps, Secrets, and Infrastructure resources to react to changes. The fix passes the informers slice to NewDeploymentController so the controller properly watches these resources, fixing both the lint error and E2E test failures.
anandkuma77
force-pushed
the
anandkuma77/cm-841-lint-fix
branch
from
c523b90 to
cf12dc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/controller/istiocsr/deployments.go:
- Line 239: The NewIrrecoverableError call in
pkg/controller/istiocsr/deployments.go currently passes issuerKind twice (once
as a valid option and again as the "configured" value); update the call so the
final formatting argument is the actual configured issuer kind (the variable
holding the configured value, e.g., issuerRef.Kind or configuredIssuerKind)
instead of reusing issuerKind, so the message shows the real configured value.
🧹 Nitpick comments (4)
pkg/controller/deployment/deployment_overrides.go (1)
196-200: Consider wrapping the error for consistency.The
NotFoundcase at line 198 wraps the error with context (configmap name), but other errors are returned unwrapped. Lister errors may not always include the resource being accessed.That said, for a lint-fix PR, the suppression is acceptable.
♻️ Optional: wrap for consistency with the NotFound case
} else if err != nil { - return err //nolint:wrapcheck // error from lister is already contextual + return fmt.Errorf("failed to get trusted CA config map %q: %w", trustedCAConfigmapName, err) }.golangci.yaml (2)
263-265: Consider using path-specific exclusions instead of global G101 disable.Disabling G101 (hardcoded credentials) globally in
gosec.excludesis broader than necessary. You already have path-specific exclusions at lines 133-140 for the actual false positives. The global exclusion makes those redundant and reduces security coverage across the entire codebase.Suggested fix: Remove the global exclusion
gosec: - excludes: - - G101 # False positives for hardcoded credentials (these are directory/volume paths) + # Path-specific exclusions for G101 false positives are defined in exclusions.rules
275-279: Global SA4006 disable may mask legitimate unused-value bugs.Disabling SA4006 ("value is never used") globally affects the entire codebase. While the comment mentions "intentional in hooks," this could mask real bugs elsewhere. Consider using
//nolint:staticcheckdirectives on specific hook functions instead, or adding a path-based exclusion if the hooks are in a specific directory.Alternative: Use path-based exclusion or inline nolint
If the unused values are specific to controller reconciliation hooks, consider either:
- Path-based exclusion in
exclusions.rules:- path: pkg/controller/.*_controller.go text: "SA4006" linters: - staticcheck
- Or keep SA4006 enabled globally and add
//nolint:staticcheck // intentional unused assignmenton specific lines.pkg/controller/istiocsr/utils.go (1)
405-428: The type assertions inside the type switch cases are redundant but satisfy the linter.Since we're inside
case *rbacv1.ClusterRole:, we already knowdesiredis*rbacv1.ClusterRole. The assertions will always succeed. This is a trade-off to satisfyforcetypeassertlint rules. Consider adding a brief comment noting this is for lint compliance if it helps future maintainers.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (29)
.golangci.yamlapi/operator/v1alpha1/certmanager_types.goapi/operator/v1alpha1/istiocsr_types.gomain.gopkg/controller/deployment/credentials_request.gopkg/controller/deployment/default_cert_manager_controller.gopkg/controller/deployment/deployment_overrides.gopkg/controller/deployment/deployment_unsupported_overrides.gopkg/controller/deployment/generic_deployment_controller.gopkg/controller/istiocsr/certificates.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/client.gopkg/controller/istiocsr/controller.gopkg/controller/istiocsr/controller_test.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/deployments_test.gopkg/controller/istiocsr/install_instiocsr_test.gopkg/controller/istiocsr/networkpolicies.gopkg/controller/istiocsr/rbacs.gopkg/controller/istiocsr/rbacs_test.gopkg/controller/istiocsr/serviceaccounts_test.gopkg/controller/istiocsr/services_test.gopkg/controller/istiocsr/test_utils.gopkg/controller/istiocsr/utils.gopkg/features/features.gopkg/operator/operatorclient/operatorclient.gopkg/operator/optionalinformer/optional_informer.gopkg/operator/setup_manager.gopkg/operator/starter.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/controller/istiocsr/install_instiocsr_test.go
- pkg/controller/istiocsr/serviceaccounts_test.go
- pkg/controller/istiocsr/controller.go
- pkg/controller/deployment/generic_deployment_controller.go
- pkg/controller/deployment/deployment_unsupported_overrides.go
- pkg/controller/istiocsr/deployments_test.go
- pkg/controller/istiocsr/test_utils.go
- pkg/operator/setup_manager.go
- pkg/controller/deployment/credentials_request.go
- pkg/controller/istiocsr/certificates.go
- pkg/controller/istiocsr/controller_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/operator/optionalinformer/optional_informer.gopkg/controller/istiocsr/certificates_test.gopkg/controller/istiocsr/networkpolicies.goapi/operator/v1alpha1/istiocsr_types.gomain.gopkg/controller/deployment/default_cert_manager_controller.gopkg/controller/istiocsr/rbacs.gopkg/controller/deployment/deployment_overrides.gopkg/operator/starter.gopkg/controller/istiocsr/client.gopkg/controller/istiocsr/deployments.gopkg/controller/istiocsr/utils.goapi/operator/v1alpha1/certmanager_types.gopkg/features/features.gopkg/controller/istiocsr/services_test.gopkg/controller/istiocsr/rbacs_test.gopkg/operator/operatorclient/operatorclient.go
🧬 Code graph analysis (3)
pkg/controller/istiocsr/networkpolicies.go (2)
api/operator/v1alpha1/certmanager_types.go (1)
NetworkPolicy(247-272)pkg/operator/applyconfigurations/operator/v1alpha1/networkpolicy.go (1)
NetworkPolicy(20-22)
pkg/controller/istiocsr/rbacs.go (2)
pkg/controller/istiocsr/controller.go (1)
Reconciler(48-55)api/operator/v1alpha1/istiocsr_types.go (1)
IstioCSR(48-64)
pkg/controller/istiocsr/utils.go (4)
api/operator/v1alpha1/groupversion_info.go (1)
SchemeGroupVersion(29-29)api/operator/v1alpha1/certmanager_types.go (1)
NetworkPolicy(247-272)pkg/operator/applyconfigurations/operator/v1alpha1/networkpolicy.go (1)
NetworkPolicy(20-22)api/operator/v1alpha1/conditions.go (2)
Ready(27-27)ReasonFailed(31-31)
🔇 Additional comments (36)
pkg/controller/istiocsr/networkpolicies.go (1)
47-50: LGTM!The safe type assertion correctly addresses the
forcetypeassertlint error. The error message helpfully includes the actual type received (got %T), which aids debugging if an incorrect asset is ever loaded.main.go (1)
29-32: LGTM!Explicitly discarding the error from
cmd.Help()with_ =is appropriate here—the program exits with code 1 immediately regardless, so there's no meaningful error handling to perform. This satisfies the linter while clearly documenting the intent.api/operator/v1alpha1/certmanager_types.go (1)
188-191: LGTM!The
nolint:godotdirective is appropriately placed to suppress the linter for the operator-sdk annotation, which is a directive comment and doesn't require a trailing period.api/operator/v1alpha1/istiocsr_types.go (1)
45-48: LGTM!Consistent with the same pattern applied in
certmanager_types.go. Thenolint:godotdirective appropriately suppresses the linter for the operator-sdk annotation.pkg/controller/deployment/deployment_overrides.go (1)
35-35: LGTM!The
nolint:gosecannotation is appropriate. G101 flags this path constant because it contains "secrets", but this is a standard OpenShift service account token mount path, not a hardcoded credential.pkg/features/features.go (1)
26-26: LGTM!The
nolint:wrapcheckdirective is appropriately justified with an inline comment explaining that the featuregate error is already contextual. This is good practice for documenting lint suppressions..golangci.yaml (2)
60-61: LGTM!The depguard disable is well-documented with a link to the relevant issue. This is a reasonable workaround for the golangci-lint v2 configuration issue.
110-112: LGTM!The path-based and text-based exclusions are reasonable:
- Test files appropriately exclude linters that would generate noise (forcetypeassert, goconst, gocritic)
- Kubebuilder/operator-sdk annotation exclusions from godot are correct
- Path-specific gosec exclusions for credential-related false positives are well-targeted
- Controller wrapcheck exclusions align with controller-runtime builder patterns
Also applies to: 120-153
pkg/operator/starter.go (2)
46-84: LGTM - wrapcheck suppressions are well-justified.The
//nolint:wrapcheckannotations on theNewForConfigerror returns are reasonable. Kubernetes client-go errors include sufficient context, and the inline explanations document the rationale.
144-152: Suppression is appropriate; minor context lifecycle note.The
//nolint:contextcheckis valid—SetupSignalHandler()is the standard controller-runtime pattern for OS signal handling. The justification clearly documents the intent.Note: The controller-runtime manager now has an independent lifecycle (OS signals) compared to other controllers using
ctx. This appears intentional per the comment, but be aware that cancelling the passedctxwon't stop this manager.pkg/controller/deployment/default_cert_manager_controller.go (1)
58-58: LGTM!The
nolint:wrapcheckannotation is appropriate here. Kubernetes client errors fromCreatealready include sufficient context about the operation and resource, making additional wrapping redundant.pkg/operator/optionalinformer/optional_informer.go (1)
57-57: LGTM!The
nolint:wrapcheckannotation is appropriate. Discovery client errors already include context about the group/version being queried.pkg/operator/operatorclient/operatorclient.go (5)
7-8: LGTM!The
slicespackage import is properly utilized forslices.Containsin theEnsureFinalizermethod.
172-178: Good refactor: avoiding builtin shadowing.Renaming
copytoupdatedis a good practice sincecopyis a Go builtin. This improves code clarity and prevents potential confusion. The nolint annotations are consistent with the rest of the file.
189-195: LGTM!Consistent variable rename and nolint annotation pattern as
UpdateOperatorSpec.
204-214: LGTM!Using
slices.Contains(line 208) is cleaner than manual iteration. The nolint annotations and finalizer handling are appropriate.
225-253: LGTM!The remaining nolint annotations follow the same consistent pattern with appropriate justifications for suppressing
wrapcheckon client/lister errors.pkg/controller/istiocsr/utils.go (5)
66-66: LGTM!Good improvement to wrap the underlying error with additional context about which resource failed the status update.
168-172: LGTM!The explicit type assertion with ok-check addresses the
forcetypeassertlint error. Panicking on failure is appropriate here since the decoder is configured with the correct scheme and a type mismatch would indicate a programming error.
180-184: LGTM!Consistent pattern applied across all decode*ObjBytes helpers - checked type assertions with descriptive panic messages on failure.
Also applies to: 192-196, 204-208, 216-220, 228-232, 240-244, 252-256
265-321: LGTM!The type assertions in
hasObjectChangedare now explicit with ok-checks. While the precedingreflect.TypeOf(desired) != reflect.TypeOf(fetched)check (line 260) should prevent mismatches, the explicit checks satisfy the linter and provide clear panic messages if assumptions are violated.
557-576: Verify the inverted control flow logic.The logic change looks correct:
- If
!ignoreProcessing(this is the oldest instance), returnnilto allow processing- Otherwise, reject the instance by updating status/annotations and returning the error
This matches the AI summary description. The early return simplifies the happy path.
pkg/controller/istiocsr/certificates_test.go (1)
37-37: LGTM!Consistent renaming of the test error sentinel from
testErrortoerrTestClientacross all mock error returns. This aligns with Go naming conventions for error variables (err prefix).Also applies to: 67-67, 116-116
pkg/controller/istiocsr/services_test.go (1)
48-48: LGTM!Consistent error sentinel renaming (
testError→errTestClient) matching the pattern across other IstioCSR test files.Also applies to: 61-61, 84-84
pkg/controller/istiocsr/rbacs_test.go (2)
31-31: LGTM!Comprehensive update of error sentinels from
testErrortoerrTestClientacross all RBAC test cases.Also applies to: 47-47, 63-63, 76-76, 90-90, 105-105, 119-119, 176-176, 217-217, 250-250, 287-287, 313-313, 336-336, 358-358, 378-378, 403-403, 427-427, 450-450, 470-470, 495-495, 519-519
258-258: LGTM!The updated
wantErrstrings correctly reflect the new error wrapping format fromutils.goline 66, which now includes"failed to update status for"in the error chain.Also applies to: 296-296
pkg/controller/istiocsr/deployments.go (3)
32-32: LGTM!Renamed error variable from
invalidIssuerRefConfigErrortoerrInvalidIssuerRefConfigfollowing Go naming conventions for error variables.
255-265: LGTM!Added explicit type assertion checks in
assertIssuerRefExists. ReturningNewIrrecoverableErroron failure is appropriate since this is a validation path where failed assertions indicate configuration issues rather than programming errors.
375-385: LGTM!Type assertion checks added in
handleIssuerBasedCA. UsingFromClientErrorhere is appropriate since this is an operational path where the issuer was already fetched but type conversion failed unexpectedly.pkg/controller/istiocsr/rbacs.go (2)
135-146: LGTM!Good rename from
newtodesired— avoids shadowing the built-innewfunction and better conveys intent.
225-236: LGTM!Consistent with the
updateClusterRoleNameInStatusrefactor above.pkg/controller/istiocsr/client.go (5)
43-59: LGTM!Consistent error wrapping with object context improves debuggability. Error chaining via
%wpreserves the original error forerrors.Is/errors.Aschecks.
61-89: LGTM!Consistent pattern across Create, Delete, and Update — extracting object key before the operation provides useful context in error messages.
96-100: Safe type assertion addresses forcetypeassert lint.The reflection-based pattern correctly handles dynamic type creation and includes explicit error handling for the type assertion. One edge case: if
objisnil,reflect.TypeOf(obj)returnsniland.Elem()will panic. In practice this would be a caller bug, but consider whether a nil-guard is warranted here.
116-134: LGTM!StatusUpdate and Patch follow the same consistent error-wrapping pattern.
136-144: LGTM!Exists correctly preserves the NotFound →
(false, nil)behavior while adding context to other errors.
mytreya-rh
left a comment
mytreya-rh
left a comment
There was a problem hiding this comment.
Thanks for the changes @anandkuma77
Dropped few comments especially on linter settings and too many inline ignores which i feel could have been re-used from centrally codified golangci.yaml
cc: @bharath-b-rh
| // CertManager is the Schema for the certmanagers API | ||
| // +operator-sdk:csv:customresourcedefinitions:displayName="CertManager" | ||
| // | ||
| //nolint:godot // operator-sdk annotation doesn't need period |
There was a problem hiding this comment.
Do we really need to inline ignore here (and few other places), given that godot linter for kubebuilder and operator-sdk annotations are excluded in .golangci.yaml
|
|
||
| // credentials for GCP. | ||
| gcpCredentialsDir = "/.config/gcloud" | ||
| gcpCredentialsDir = "/.config/gcloud" //nolint:gosec // G101: path constant, not a credential |
There was a problem hiding this comment.
Two points (for this and other G101 inline exclusions):
- In .golangci.yaml we have globally excluded G101 check from gosec. Thus, we perhaps don't need the explicit inline exclusion right?
- Can we instead not exclude the G101 check but change the variable name for example to gcpConfigDir so that the variable doesn't get flagged?
.golangci.yaml
Outdated
| chain-split-dots: false | ||
| chain-split-dots: false | ||
|
|
||
| linters-settings: |
There was a problem hiding this comment.
This (linters-settings) seems to be specific to legacy v1 style: https://golangci.github.io/legacy-v1-doc/usage/linters/#linters-configuration and will not work for v2: https://golangci-lint.run/docs/linters/configuration/ (where linters.settings i.e. settings struct under linters struct is expected)
Please see the difference (between v1 and v2 linter settings examples), as the current linters-settings i think is not being used at all.
Found this while checking why we have inline wrap ignores despite wrapcheck setting here.
| return fmt.Errorf("(Retrying) cloud secret %q doesn't exist due to %w", secretName, err) | ||
| } else if err != nil { | ||
| return err | ||
| return err //nolint:wrapcheck // error from lister is already contextual |
There was a problem hiding this comment.
please see comment golangci lint v1 vs v2 for linter-settings.. perhaps we will not need this explicit inline ignore after we fix the structure there, and if needed add some additional patterns to cover for common cases where wrapping is not needed. Same comment for all other similar inline ignores of wrapcheck
There was a problem hiding this comment.
Thanks, updating setting to V2 solved this.
…int comments - Migrate .golangci.yaml from v1 to v2 format - Remove all //nolint:wrapcheck comment - Fix G101 lint errors by renaming credential-related variables: - Update godot linter configuration
anandkuma77
force-pushed
the
anandkuma77/cm-841-lint-fix
branch
from
cf12dc3 to
3bbe34a
Compare
- Remove nolint comments from istiocsr controller and client - Update variable names in credentials_request.go - Clean up deployment_overrides.go - Remove unused comments from istiocsr_types.go
|
@anandkuma77: all tests passed! 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. |
| } | ||
| } | ||
| deployment = unsupportedConfigOverrides(deployment, cfg) | ||
| deployment = unsupportedConfigOverrides(deployment, cfg) //nolint:staticcheck,wastedassign // SA4006: deployment is modified in place, assignment kept for clarity |
There was a problem hiding this comment.
nit: since deployment is modified in-place and the same is not used for any other purpose after this call, do we need to reassign?
| deployment = unsupportedConfigOverrides(deployment, cfg) //nolint:staticcheck,wastedassign // SA4006: deployment is modified in place, assignment kept for clarity | |
| unsupportedConfigOverrides(deployment, cfg) |
| - path: pkg/operators | ||
| linters: | ||
| - all | ||
| # Exclude hack directory from some linters |
There was a problem hiding this comment.
I think these can be simplified, but requires the golinter to be updated I guess. Lets check in the next iteration of linter. Especially operator-sdk and kubebuilder doesn't need to be excluded explicitly.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anandkuma77, bharath-b-rh 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
6 participants
Summary
Fixes multiple golangci-lint errors to ensure the codebase passes all linting checks.
Changes