Update project linting to adhere to golangci-lint v2 defaults#1636
Conversation
WalkthroughThe changes across the codebase primarily refactor conditional logic from if-else chains to switch statements, simplify and correct field accesses for Kubernetes object metadata, and standardize resource cleanup and environment variable setting by introducing utility functions ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Util as util
participant OS as OS/Resource
Test->>Util: SafeSetEnv("VAR", "value")
Util->>OS: Set environment variable
OS-->>Util: Return error or success
Util-->>Test: Log error if any
Test->>Util: SafeClose(resource, "error message")
Util->>OS: resource.Close()
OS-->>Util: Return error or success
Util-->>Test: Log error if any
sequenceDiagram
participant Controller as Controller
participant K8sObj as K8s Object
Controller->>K8sObj: Access .Annotations directly
Note right of K8sObj: Instead of .ObjectMeta.Annotations
sequenceDiagram
participant Provisioner as Provisioner
participant OB as ObjectBucket
Provisioner->>OB: Access .Spec.Endpoint
Note right of OB: Instead of .Spec.Connection.Endpoint
OB-->>Provisioner: Return BucketName, AdditionalConfigData
These diagrams illustrate the introduction of utility functions for safer resource handling, the direct access of Kubernetes object fields, and the updated ObjectBucket endpoint access pattern. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (27)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (24)
⏰ Context from checks skipped due to timeout of 90000ms (13)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
pkg/system/system.go (1)
281-319: Guard against potential nil-pointer onEndpoints.ResourcesNice move to a
switch, but theendpointsbranch does not guarantee thatsys.Spec.Endpoints.Resourcesis non-nil when the spec already exists.
If the user supplied onlyminCount/maxCountvia flags (or the CR arrives from an earlier version),Resourcescan staynil, leading to a panic when the code below writes tocomponent.Requests/component.Limits.case "endpoints": if sys.Spec.Endpoints == nil { sys.Spec.Endpoints = &nbv1.EndpointsSpec{ MinCount: viper.GetInt32("resources.endpoints.minCount"), MaxCount: viper.GetInt32("resources.endpoints.maxCount"), Resources: &corev1.ResourceRequirements{ Requests: corev1.ResourceList{}, Limits: corev1.ResourceList{}, }, } } else { + // Ensure Resources is initialized before dereference + if sys.Spec.Endpoints.Resources == nil { + sys.Spec.Endpoints.Resources = &corev1.ResourceRequirements{ + Requests: corev1.ResourceList{}, + Limits: corev1.ResourceList{}, + } + } if viper.IsSet("resources.endpoints.minCount") { sys.Spec.Endpoints.MinCount = viper.GetInt32("resources.endpoints.minCount") } if viper.IsSet("resources.endpoints.maxCount") { sys.Spec.Endpoints.MaxCount = viper.GetInt32("resources.endpoints.maxCount") } } component = sys.Spec.Endpoints.Resources
🧹 Nitpick comments (1)
pkg/system/system.go (1)
769-769: Local variable should be lower-camel for consistency
var NooBaaDB = r.NooBaaPostgresDBintroduces an exported-style identifier inside a function. While valid, it tripsgolint/reviverules (var-naming) and is inconsistent with the rest of the file (coreApp,cnpgCluster, etc.). Consider:noobaaDB := r.NooBaaPostgresDB(or restore the explicit type if that was intentional).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
pkg/backingstore/reconciler.go(4 hunks)pkg/bucketclass/bucketclass.go(5 hunks)pkg/cli/cli_test.go(1 hunks)pkg/controller/cephcluster/cephcluster_controller.go(2 hunks)pkg/cosi/cosi_bucket_class.go(1 hunks)pkg/cosi/cosi_cli_test.go(1 hunks)pkg/cosi/cosi_test.go(1 hunks)pkg/cosi/provisioner.go(3 hunks)pkg/diagnostics/analyze.go(2 hunks)pkg/diagnostics/collect.go(1 hunks)pkg/diagnostics/dbdump.go(1 hunks)pkg/namespacestore/reconciler.go(1 hunks)pkg/nb/rpc_http.go(1 hunks)pkg/obc/provisioner.go(2 hunks)pkg/olm/olm.go(1 hunks)pkg/system/autoscaler.go(1 hunks)pkg/system/db_reconciler.go(1 hunks)pkg/system/phase1_verifying.go(2 hunks)pkg/system/phase2_creating.go(4 hunks)pkg/system/phase4_configuring.go(4 hunks)pkg/system/system.go(2 hunks)pkg/util/kms/kms_ibm_kp.go(2 hunks)pkg/util/kms/kms_kmip_storage.go(4 hunks)pkg/util/kms/kms_vault.go(1 hunks)pkg/util/util.go(9 hunks)pkg/validations/backingstore_validations.go(1 hunks)pkg/validations/common_bucket_validations.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: golangci-lint
- GitHub Check: run-cli-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-hac-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-kmip-test
🔇 Additional comments (44)
pkg/util/util.go (3)
2351-2367: Excellent addition of generic utility functions for safer resource handling.The
SafeCloseandSafeSetEnvfunctions provide a consistent, safe approach to resource cleanup and environment variable setting:
SafeCloseuses Go generics effectively to work with any closeable resource- Both functions log errors instead of silently ignoring them
- This standardizes error handling patterns across the codebase
809-809: Good adoption of SafeClose for consistent resource cleanup.The replacement of direct
Close()calls withSafeClose()improves error handling by ensuring cleanup errors are logged rather than silently ignored. Each usage includes appropriate descriptive error messages.Also applies to: 817-817, 838-838, 1394-1394, 1453-1453, 1456-1456, 1491-1491, 1510-1510
1272-1272: Minor improvements enhance code clarity.Good changes:
- Line 1272: Consistent lowercase in error message
- Line 1478:
ReplaceAllis more explicit thanReplacewith count parameterAlso applies to: 1478-1478
pkg/validations/common_bucket_validations.go (2)
44-48: Minor comment formatting improvement.The comment indentation adjustment improves code readability.
85-85: Good simplification of boolean assignment.The direct assignment
IsExternalRPCConnection := util.IsTestEnv() || isCLIis cleaner than an if-else block and maintains the same logic.pkg/system/autoscaler.go (1)
354-354: Good simplification of boolean logic.Converting the logic to
!meta.IsNoMatchError(err) && !runtime.IsNotRegisteredError(err)makes it clearer that validation passes when neither error condition is true.pkg/nb/rpc_http.go (1)
48-48: Consistent adoption of SafeClose for HTTP response cleanup.The replacement of direct
Close()withSafeClose()aligns with the safer resource cleanup pattern established in this PR and ensures any errors during HTTP response body closing are properly logged.pkg/cosi/cosi_cli_test.go (1)
19-19: LGTM! Improved error handling for environment variable setting.The change from direct
os.Setenvtoutil.SafeSetEnvaligns with the project's standardization effort and ensures any environment variable setting errors are properly logged during test setup.pkg/diagnostics/collect.go (1)
119-119: LGTM! Enhanced resource cleanup with error logging.The replacement of direct
defer outfile.Close()withutil.SafeCloseensures that file closure errors are properly logged with context, which is especially valuable in diagnostics collection where resource issues could indicate underlying problems.pkg/util/kms/kms_ibm_kp.go (2)
43-43: LGTM! Standardized environment variable setting with error logging.The change from direct
os.Setenvtoutil.SafeSetEnvensures that errors setting the IBM instance ID environment variable are properly logged, which aids in debugging KMS configuration issues.
71-71: LGTM! Improved error handling for sensitive configuration.The use of
util.SafeSetEnvfor caching API keys ensures that environment variable setting errors are logged, which is particularly important when handling sensitive KMS credentials.pkg/cosi/cosi_test.go (1)
23-23: LGTM! Consistent error handling for test environment setup.The replacement of direct
os.Setenvwithutil.SafeSetEnvmaintains consistency with the project's standardized approach to environment variable setting and ensures test setup errors are properly logged.pkg/cli/cli_test.go (1)
21-21: LGTM! Standardized test environment configuration.The change from direct
os.Setenvtoutil.SafeSetEnvcompletes the consistent application of safer environment variable handling across the CLI test suite.pkg/util/kms/kms_vault.go (1)
163-163: Excellent error handling improvement.Replacing the direct
file.Close()call withutil.SafeClose()ensures that any errors during file closure are properly logged with a descriptive message, rather than being silently ignored.pkg/diagnostics/dbdump.go (1)
97-97: Consistent error handling improvement.The change to use
util.SafeClose()aligns with the broader pattern across the codebase to ensure file closure errors are properly logged with descriptive messages.pkg/system/db_reconciler.go (1)
258-258: Good simplification of annotation access.Using
r.NooBaa.Annotationsdirectly instead ofr.NooBaa.ObjectMeta.Annotationsis more idiomatic and concise while maintaining the same functionality.pkg/diagnostics/analyze.go (2)
477-477: Consistent with the resource cleanup pattern.The change to use
util.SafeClose()maintains consistency with the error handling improvements applied throughout the codebase.
421-421: Good code hygiene improvement.Removing trailing whitespace improves code cleanliness and aligns with standard linting practices.
pkg/controller/cephcluster/cephcluster_controller.go (2)
96-97: Consistent annotation access simplification.Using
bs.Annotationsdirectly instead ofbs.ObjectMeta.Annotationsfollows the same simplification pattern applied throughout the codebase, improving readability while maintaining functionality.
37-37: Minor formatting improvement.The field alignment adjustment improves code readability and follows consistent formatting standards.
pkg/system/phase1_verifying.go (2)
61-62: LGTM: Standardized environment variable setting with error handling.The replacement of direct
os.Setenvcalls withutil.SafeSetEnvimproves error handling consistency across the codebase by providing centralized error logging for environment variable operations.
236-236: LGTM: Improved database connection cleanup with error logging.The use of
util.SafeCloseinstead of directdefer db.Close()ensures that any errors during connection closure are properly logged with a descriptive message, enhancing debugging capabilities.pkg/namespacestore/reconciler.go (1)
558-558: LGTM: Simplified field access improves readability.The direct access to
secret.OwnerReferencesinstead ofsecret.ObjectMeta.OwnerReferencesis valid sinceOwnerReferencesis embedded and directly accessible on Kubernetes objects. This change enhances code conciseness without functional impact.pkg/cosi/cosi_bucket_class.go (1)
232-239: LGTM: Switch statement improves readability and maintainability.The conversion from if-else chain to switch statement for deletion policy handling enhances code clarity and reduces cyclomatic complexity. The logic is preserved correctly with proper handling of "delete", "retain", and invalid cases.
pkg/util/kms/kms_kmip_storage.go (2)
138-138: LGTM: Enhanced error handling for connection cleanup during failures.Replacing direct
conn.Close()calls withutil.SafeClose()in error paths ensures that connection closure failures are properly logged with descriptive messages, improving debugging capabilities for KMIP connection issues.Also applies to: 144-144
276-276: LGTM: Consistent connection cleanup with error logging in deferred calls.The use of
util.SafeClose()in deferred connection cleanup ensures that any errors during connection closure are logged with meaningful context messages, maintaining consistency with the broader codebase error handling patterns.Also applies to: 349-349, 410-410
pkg/bucketclass/bucketclass.go (2)
661-702: LGTM: Switch statement improves readability in namespace store mapping logic.The conversion from if-else chain to switch statement for handling namespace policy types (
Single,Multi,Cache) enhances code clarity and reduces cyclomatic complexity while preserving the exact same logic for determining bucket class reconciliation requirements.
753-779: LGTM: Switch statement enhances namespace bucket info creation logic.The refactoring to use a switch statement for namespace policy type handling improves code readability and maintainability. The logic correctly preserves the behavior for creating appropriate namespace bucket configurations based on policy type (
Single,Multi,Cache).pkg/cosi/provisioner.go (3)
72-74: Excellent error handling improvement!Adding proper error checking and logging for socket file removal is a great defensive programming practice. This prevents silent failures and provides visibility into potential issues during startup.
245-245: Clean boolean assignment simplification.Directly assigning the result of
util.IsTestEnv()is more concise and idiomatic than using an intermediate variable declaration.
286-286: Consistent boolean assignment improvement.This matches the same simplification pattern applied elsewhere in the function, maintaining consistency.
pkg/system/phase2_creating.go (3)
376-376: Improved Kubernetes object field access patterns.Using
r.CoreApp.Kindinstead ofr.CoreApp.TypeMeta.Kindis more idiomatic and aligns with standard Kubernetes API patterns. This simplification improves readability while maintaining the same functionality.Also applies to: 385-385
828-828: Corrected object name access pattern.Using direct field access (
cephObjectStoreList.Items[0].Name) instead of accessing through the embeddedObjectMetafield is the preferred and more idiomatic approach in Kubernetes APIs.
1440-1440: Simplified StorageClass annotations access.Direct access to
sc.Annotationsis cleaner and more idiomatic than accessing throughsc.ObjectMeta.Annotations. This change aligns with the consistent pattern applied throughout the codebase.pkg/validations/backingstore_validations.go (1)
264-271: Excellent refactoring to switch statement.Converting the if-else chain to a switch statement significantly improves readability while maintaining identical logic and error handling. This change aligns perfectly with the PR's goal of adhering to golangci-lint v2 standards and reducing condition complexity.
pkg/olm/olm.go (1)
745-756: Well-executed conditional logic refactoring.The conversion from if-else chain to switch statement improves code clarity and readability while preserving the exact same logic for determining CRD ownership and requirements. This change exemplifies the consistent refactoring approach applied throughout the PR to enhance maintainability.
pkg/obc/provisioner.go (3)
397-397: LGTM! Bucket name access updated consistently.The bucket name access has been correctly updated to use the new direct endpoint access pattern.
406-408: LGTM! Additional config data access updated consistently.The additional config data access has been correctly updated to use the new direct endpoint access pattern, maintaining consistency with the schema changes.
394-396: Verify ObjectBucketSpec and Endpoint struct definitions for schema consistency#!/bin/bash # Locate the ObjectBucketSpec definition echo "Searching for ObjectBucketSpec definition:" rg -n "type ObjectBucketSpec" --type go echo -e "\nSearching for Endpoint struct definition:" rg -n "type Endpoint" --type gopkg/system/phase4_configuring.go (3)
171-212: Excellent refactoring from if-else chain to switch statement.The conversion from
if-else ifstatements to aswitchstatement onres1.Statesignificantly improves code readability and aligns with Go best practices. The logic flow remains identical while being more maintainable.
298-298: LGTM! Annotation access simplified correctly.The direct access to
r.NooBaa.Annotationsinstead ofr.NooBaa.ObjectMeta.Annotationsis valid and more concise, as theAnnotationsfield is accessible directly on Kubernetes objects.
1434-1437: LGTM! Consistent annotation access pattern.The direct access to
r.DefaultBackingStore.Annotationsmaintains consistency with the annotation access pattern used throughout the file and is correctly implemented.pkg/backingstore/reconciler.go (2)
309-320: Ownership-check refactor is soundSwitching to
secret.OwnerReferencesleverages struct embedding and removes verbosity without changing behaviour. ✅
1015-1017: Minor refactor: direct annotation access looks fineAccessing
r.BackingStore.Annotationsdirectly eliminates redundantObjectMetadereference and the nil-check guards against panics.
aayushchouhan09
left a comment
There was a problem hiding this comment.
Please squash your commits before merging. Thanks!
78ccffc to
b5fd933
Compare
Signed-off-by: Ben <belimele@redhat.com>
b5fd933 to
7b240ba
Compare
Explain the changes
1. General lint-driven refactors
if…elsechains (e.g. signature-version checks, bucket-class mapping, CSV grouping) intoswitchblocks for clarity and fewer conditions.secret.ObjectMeta.OwnerReferencesandsc.ObjectMeta.Annotationsfor the shortersecret.OwnerReferencesandsc.Annotations.2. Errcheck fixes & cleanup
defer file.Close()(and similar) with closures that log any close-errors, unified under a newutil.SafeClosehelper.os.Setenvcalls throughutil.SafeSetEnvto remove repeated error-check boilerplate.3. New helpers
SafeClose[T interface{ Close() error }](resource T, errorMsg string)Centralizes error logging on
Close()failures, leveraging Go 1.18+ generics.SafeSetEnv(name, value string)Wraps
os.Setenvcalls, logging any errors to keep tests and mocks clean.4. Small goodies
osin several files).return !(A || B)→return !A && !B).var NooBaaDB = …instead of specifying*StatefulSet).Summary by CodeRabbit
Refactor
Bug Fixes
Chores