refactor(core): update and address multiple golangci-lint issues#1836
Merged
refactor(core): update and address multiple golangci-lint issues#1836
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
internal/controller/cluster/cluster_controller.go (1)
76-76: Add guarded type assertions for defensive consistency.Lines 76 and 294 use unchecked assertions. While the current design (caller passes typed template to
lifecycle.Reconcile) guarantees type safety at runtime, other reconcilers in the codebase (e.g.,OrganizationReconciler,TeamRoleBindingReconciler,TeamController) use guarded assertions. Adding type checks makes the code more resilient to future refactoring and improves consistency across the codebase.Proposed diff
func (r *RemoteClusterReconciler) EnsureCreated(ctx context.Context, resource lifecycle.RuntimeObject) (ctrl.Result, lifecycle.ReconcileResult, error) { - cluster := resource.(*greenhousev1alpha1.Cluster) + cluster, ok := resource.(*greenhousev1alpha1.Cluster) + if !ok { + return ctrl.Result{}, lifecycle.Failed, fmt.Errorf("unexpected runtime object type %T", resource) + } if cluster.Spec.AccessMode != greenhousev1alpha1.ClusterAccessModeDirect { return ctrl.Result{}, lifecycle.Failed, nil } func (r *RemoteClusterReconciler) EnsureDeleted(ctx context.Context, resource lifecycle.RuntimeObject) (ctrl.Result, lifecycle.ReconcileResult, error) { - cluster := resource.(*greenhousev1alpha1.Cluster) + cluster, ok := resource.(*greenhousev1alpha1.Cluster) + if !ok { + return ctrl.Result{}, lifecycle.Failed, fmt.Errorf("unexpected runtime object type %T", resource) + } c := cluster.Status.GetConditionByType(greenhousev1alpha1.KubeConfigValid) if c != nil && c.IsFalse() { return ctrl.Result{}, lifecycle.Success, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/cluster/cluster_controller.go` at line 76, Replace the unchecked type assertions for resource to a guarded assertion to mirror other reconcilers: instead of resource.(*greenhousev1alpha1.Cluster) use a two-value assertion (cluster, ok := resource.(*greenhousev1alpha1.Cluster)) and handle the false branch by logging an error and returning an appropriate error (or requeue) from the current reconcile path; update both occurrences where the unchecked assertion is used (the cluster variable creation and the later assertion around line 294) so they follow the same defensive pattern as OrganizationReconciler/TeamController.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yaml:
- Around line 19-20: The goimports configuration in .golangci.yaml incorrectly
lists local-prefixes as "github.com/sapcc/keppel"; update the local-prefixes
entry (the local-prefixes key under goimports) to the correct repository host
path "github.com/cloudoperators/greenhouse" so project imports are treated as
local and grouped properly by goimports.
- Around line 1-5: Remove the duplicate SPDX header block so only one SPDX
copyright and license pair remains; specifically, keep a single
"SPDX-FileCopyrightText" / "SPDX-License-Identifier" pair (e.g., the 2026 block)
and delete the redundant 2024 block lines ("SPDX-FileCopyrightText: 2024 ..."
and its corresponding "SPDX-License-Identifier: Apache-2.0") so the file
contains only one SPDX header.
In `@e2e/shared/utils.go`:
- Line 15: The Log function currently assumes args has at least one element and
that args[0] is a string, causing panics; update Log to first check len(args) >
0 before touching args[0] and avoid direct string cast—use a safe type assertion
or convert the first element with fmt.Sprint when present, e.g. if len(args) ==
0 prepend the prefix as a standalone message or if args[0] is not a string
replace args[0] with fmt.Sprint(args[0]) before prefixing; locate the code using
args[0] in the Log function in e2e/shared/utils.go and apply this guard and safe
conversion.
In `@internal/local/setup/manifest.go`:
- Around line 132-133: The code currently does unchecked type assertions on
deployment["metadata"] and metadata["name"] (metadata :=
deployment["metadata"].(map[string]any); name := metadata["name"].(string)),
which can panic for malformed manifests; replace these with defensive checks:
verify deployment["metadata"] exists and is a map[string]any, verify
metadata["name"] exists and is a string, and if any check fails return a
descriptive typed error (rather than panic). Update the surrounding function to
propagate that error (use the existing error type or define a clear error value)
so callers can handle malformed deployment entries safely.
In `@internal/local/setup/webhook.go`:
- Around line 117-118: The direct type assertion annAny.(map[string]any) can
panic for unexpected manifest shapes; change it to use the safe "v, ok :=
annAny.(map[string]any)" pattern and only assign annotations = v when ok is true
(otherwise leave annotations as nil or handle accordingly) in the same block
where annAny is read so it mirrors the safe metadata handling used around lines
106-112.
- Around line 387-389: The code directly asserts addr.(*net.IPNet) which can
panic; change the loop over Interface.Addrs() to use a safe type assertion
(e.g., ipnet, ok := addr.(*net.IPNet')) and skip addresses where ok is false,
optionally also handle other address types like *net.IPAddr by checking their
concrete type before calling To4(); update the block that logs and returns the
docker0 IPv4 to only run when a valid ipnet (or equivalent) is obtained so no
panic occurs.
In `@internal/local/utils/utils.go`:
- Line 35: In the Log function guard access to args[0] before using a string
assertion: check len(args) > 0; if true, do a safe type assertion (s, ok :=
args[0].(string)) and, if ok, set args[0] = "===== 🤖 "+s; otherwise replace
args[0] with "===== 🤖 "+fmt.Sprint(args[0]); if len(args) == 0, prepend a new
first argument "===== 🤖 " (e.g., args = append([]interface{}{"===== 🤖 "},
args...)). This ensures no panic from missing or non-string first arguments
while preserving the original Log behavior.
---
Nitpick comments:
In `@internal/controller/cluster/cluster_controller.go`:
- Line 76: Replace the unchecked type assertions for resource to a guarded
assertion to mirror other reconcilers: instead of
resource.(*greenhousev1alpha1.Cluster) use a two-value assertion (cluster, ok :=
resource.(*greenhousev1alpha1.Cluster)) and handle the false branch by logging
an error and returning an appropriate error (or requeue) from the current
reconcile path; update both occurrences where the unchecked assertion is used
(the cluster variable creation and the later assertion around line 294) so they
follow the same defensive pattern as OrganizationReconciler/TeamController.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11e5c2c2-eda7-4063-ba0a-de5d4d187ab0
📒 Files selected for processing (29)
.golangci.yamlcmd/service-proxy/instrumentation.goe2e/shared/utils.gogo.modinternal/clientutil/patch.gointernal/common/url.gointernal/controller/catalog/artifact.gointernal/controller/catalog/catalog_controller.gointernal/controller/cluster/bootstrap_controller.gointernal/controller/cluster/cluster_controller.gointernal/controller/organization/organization_controller_test.gointernal/controller/organization/rbac.gointernal/controller/organization/service_proxy.gointernal/controller/plugin/plugin_controller.gointernal/controller/plugin/pluginpreset_controller.gointernal/controller/plugindefinition/cluster_plugindefinition_controller.gointernal/controller/plugindefinition/plugindefinition_controller.gointernal/dex/connector.gointernal/features/features_test.gointernal/flux/helm_release_builder.gointernal/flux/utils.gointernal/local/klient/kind.gointernal/local/setup/manager.gointernal/local/setup/manifest.gointernal/local/setup/webhook.gointernal/local/utils/utils.gointernal/test/util.gopkg/lifecycle/context.gopkg/lifecycle/propagation_test.go
IvoGoman
force-pushed
the
refactor/golangci-lint
branch
2 times, most recently
from
c4e7d6e to
76093fc
Compare
IvoGoman
force-pushed
the
refactor/golangci-lint
branch
from
c4e7d6e to
76093fc
Compare
IvoGoman
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/local/setup/webhook.go (2)
117-118:⚠️ Potential issue | 🟠 MajorAnnotations assertion is still panic-prone for unexpected manifest shape.
Line 117 still directly asserts
annAny.(map[string]any)without anokcheck.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/local/setup/webhook.go` around lines 117 - 118, The direct type assertion annAny.(map[string]any) is panic-prone; update the code around the assignment to safely type-assert by using the "comma ok" form (e.g., val, ok := annAny.(map[string]any)) and only set annotations = val when ok is true; if the assertion fails, handle the unexpected manifest shape by logging a warning or skipping/initializing annotations appropriately instead of panicking so annAny and annotations are robust to different manifest structures.
387-389:⚠️ Potential issue | 🟠 MajorAddress type assertion is still unsafe and can panic.
Line 387 still assumes every
net.Addris*net.IPNet; this should remain guarded with a safe type assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/local/setup/webhook.go` around lines 387 - 389, The current type assertion on addr is unsafe and can panic; change the block that reads `if ipv4 := addr.(*net.IPNet).IP.To4(); ipv4 != nil { ... }` to first perform a guarded assertion like `ipnet, ok := addr.(*net.IPNet)` and only call `ipnet.IP.To4()` when ok is true, then log and return `ipv4.String()` as before; ensure the outer loop or function continues safely when the assertion fails (no panic) so non-*net.IPNet addresses are skipped.internal/local/setup/manifest.go (1)
132-133:⚠️ Potential issue | 🟠 MajorUnchecked assertions can still panic on malformed deployment manifests.
Line 132 and Line 133 still use direct assertions (
map[string]anyandstring) without guards, so malformed input can crash setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/local/setup/manifest.go` around lines 132 - 133, The code currently uses unchecked type assertions on deployment and its fields (deployment, metadata := deployment["metadata"].(map[string]any), name := metadata["name"].(string)) which can panic on malformed manifests; update the logic in manifest.go to use safe type assertions (the "ok" comma-ok form) for deployment["metadata"] and metadata["name"], validate their presence and types, and return or propagate a descriptive error (or skip the entry) instead of allowing a panic so callers of the function can handle malformed inputs.
🧹 Nitpick comments (1)
.golangci.yaml (1)
199-202: Remove or justify stalereviveexclusion.Line 201 excludes
revive, butreviveis not enabled in the linter set. Keeping dead exclusions makes the config harder to reason about.♻️ Proposed cleanup
- linters: - bodyclose - - revive path: _test\.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yaml around lines 199 - 202, The config contains a stale exclusion: remove the unused "revive" entry from the linters list under the per-path rule that targets path: _test\.go (the linters: - bodyclose - revive block) or alternatively enable the revive linter globally if you intend to keep that exclusion; update the linters array to only include enabled linters (e.g., remove the "- revive" line) so the .golangci.yaml no longer contains a dead exclusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yaml:
- Line 94: Update the typo in the forbidigo remediation message: locate the msg
value that currently contains "gopk.in/square/go-jose" and change it to
"gopkg.in/square/go-jose" so the guidance points to the correct package path
(the field to edit is the msg entry in .golangci.yaml that references go-jose).
---
Duplicate comments:
In `@internal/local/setup/manifest.go`:
- Around line 132-133: The code currently uses unchecked type assertions on
deployment and its fields (deployment, metadata :=
deployment["metadata"].(map[string]any), name := metadata["name"].(string))
which can panic on malformed manifests; update the logic in manifest.go to use
safe type assertions (the "ok" comma-ok form) for deployment["metadata"] and
metadata["name"], validate their presence and types, and return or propagate a
descriptive error (or skip the entry) instead of allowing a panic so callers of
the function can handle malformed inputs.
In `@internal/local/setup/webhook.go`:
- Around line 117-118: The direct type assertion annAny.(map[string]any) is
panic-prone; update the code around the assignment to safely type-assert by
using the "comma ok" form (e.g., val, ok := annAny.(map[string]any)) and only
set annotations = val when ok is true; if the assertion fails, handle the
unexpected manifest shape by logging a warning or skipping/initializing
annotations appropriately instead of panicking so annAny and annotations are
robust to different manifest structures.
- Around line 387-389: The current type assertion on addr is unsafe and can
panic; change the block that reads `if ipv4 := addr.(*net.IPNet).IP.To4(); ipv4
!= nil { ... }` to first perform a guarded assertion like `ipnet, ok :=
addr.(*net.IPNet)` and only call `ipnet.IP.To4()` when ok is true, then log and
return `ipv4.String()` as before; ensure the outer loop or function continues
safely when the assertion fails (no panic) so non-*net.IPNet addresses are
skipped.
---
Nitpick comments:
In @.golangci.yaml:
- Around line 199-202: The config contains a stale exclusion: remove the unused
"revive" entry from the linters list under the per-path rule that targets path:
_test\.go (the linters: - bodyclose - revive block) or alternatively enable the
revive linter globally if you intend to keep that exclusion; update the linters
array to only include enabled linters (e.g., remove the "- revive" line) so the
.golangci.yaml no longer contains a dead exclusion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d686230-f33e-44f1-897a-6e855945242b
📒 Files selected for processing (30)
.golangci.yamlcmd/service-proxy/instrumentation.goe2e/shared/utils.gogo.modinternal/clientutil/patch.gointernal/common/url.gointernal/controller/catalog/artifact.gointernal/controller/catalog/catalog_controller.gointernal/controller/cluster/bootstrap_controller.gointernal/controller/cluster/cluster_controller.gointernal/controller/organization/organization_controller_test.gointernal/controller/organization/rbac.gointernal/controller/organization/service_proxy.gointernal/controller/plugin/plugin_controller.gointernal/controller/plugin/pluginpreset_controller.gointernal/controller/plugindefinition/cluster_plugindefinition_controller.gointernal/controller/plugindefinition/plugindefinition_controller.gointernal/dex/connector.gointernal/features/features_test.gointernal/flux/helm_release_builder.gointernal/flux/utils.gointernal/local/klient/kind.gointernal/local/setup/manager.gointernal/local/setup/manifest.gointernal/local/setup/webhook.gointernal/local/utils/utils.gointernal/test/util.gopkg/lifecycle/context.gopkg/lifecycle/propagation.gopkg/lifecycle/propagation_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/clientutil/patch.go
- internal/controller/organization/rbac.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/controller/organization/service_proxy.go
- internal/features/features_test.go
- internal/controller/plugin/pluginpreset_controller.go
- internal/controller/plugindefinition/plugindefinition_controller.go
- internal/dex/connector.go
- internal/controller/plugin/plugin_controller.go
- internal/controller/organization/organization_controller_test.go
- internal/local/utils/utils.go
- go.mod
- internal/flux/helm_release_builder.go
- internal/local/setup/manager.go
IvoGoman
force-pushed
the
refactor/golangci-lint
branch
from
76093fc to
783c948
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/shared/utils.go (1)
15-15:⚠️ Potential issue | 🔴 CriticalPrevent panic in
Logfor empty/non-string arguments.At Line 15,
args[0]andargs[0].(string)can panic whenargsis empty or first arg is not astring. Please guard length and stringify safely.Proposed fix
import ( "bytes" + "fmt" "io" @@ func Log(args ...any) { - args[0] = "===== 🤖 " + args[0].(string) - klog.InfoDepth(1, args...) + if len(args) == 0 { + klog.InfoDepth(1, "===== 🤖") + return + } + msgArgs := append([]any(nil), args...) + msgArgs[0] = "===== 🤖 " + fmt.Sprint(msgArgs[0]) + klog.InfoDepth(1, msgArgs...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/shared/utils.go` at line 15, The Log function currently does args[0] = "===== 🤖 " + args[0].(string) which panics if args is empty or args[0] isn't a string; update the code in the Log function to first check len(args) and if zero, prepend a new first element with the decorated string, otherwise replace args[0] with "===== 🤖 "+fmt.Sprint(args[0]) (using fmt.Sprint to safely stringify any type instead of a direct string type assertion) so that empty slices and non-string first arguments are handled without panicking.
🧹 Nitpick comments (2)
internal/controller/organization/rbac.go (1)
176-180: Consider defining aroleKindconstant for consistency.
clusterRoleKindis now a constant, but"Role"remains a string literal here (line 178) and on line 360. For consistency, consider adding aroleKind = "Role"constant alongsideclusterRoleKind.♻️ Suggested refactor
const ( clusterRoleKind = "ClusterRole" + roleKind = "Role" )Then update usages:
roleBinding.RoleRef = rbacv1.RoleRef{ APIGroup: rbacv1.GroupName, - Kind: "Role", + Kind: roleKind, Name: roleBindingName, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/organization/rbac.go` around lines 176 - 180, Add a new constant roleKind = "Role" (to mirror existing clusterRoleKind) and replace the string literal "Role" wherever used (for example in the RoleRef assignment in roleBinding.RoleRef and the other occurrence referenced) with the new roleKind constant; update references in functions and structs that currently use the literal so all role kinds use the roleKind symbol for consistency..golangci.yaml (1)
199-202: Remove stalereviveexclusion.With
linters.default: noneandrevivenot in the enabled linters list, the exclusion at line 201 has no effect—revivedoesn't run, so there are no issues to filter. Removing it prevents future maintenance confusion.Proposed cleanup
- linters: - bodyclose - - revive path: _test\.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yaml around lines 199 - 202, Remove the stale `revive` exclusion from the linters list in the `.golangci.yaml` configuration: edit the `linters` block associated with `path: _test\.go` and delete the `revive` entry (leaving `bodyclose` or other valid linters intact); this aligns the file with `linters.default: none` and avoids a no-op exclusion for a linter that is not enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yaml:
- Around line 155-161: Remove the gosec suppressions for G701, G702, G703, G704,
G705, and G706 from .golangci.yaml and ensure the repository uses gosec v2.24.0
or newer in the CI/tooling configuration so the false positives no longer occur;
specifically delete the lines listing "- G701" through "- G706" and bump the
gosec/golangci-lint tool version in your CI/workflow or toolchain config to >=
v2.24.0 (or update gosec in any dependency manifest) before re-enabling these
checks.
---
Duplicate comments:
In `@e2e/shared/utils.go`:
- Line 15: The Log function currently does args[0] = "===== 🤖 " +
args[0].(string) which panics if args is empty or args[0] isn't a string; update
the code in the Log function to first check len(args) and if zero, prepend a new
first element with the decorated string, otherwise replace args[0] with "=====
🤖 "+fmt.Sprint(args[0]) (using fmt.Sprint to safely stringify any type instead
of a direct string type assertion) so that empty slices and non-string first
arguments are handled without panicking.
---
Nitpick comments:
In @.golangci.yaml:
- Around line 199-202: Remove the stale `revive` exclusion from the linters list
in the `.golangci.yaml` configuration: edit the `linters` block associated with
`path: _test\.go` and delete the `revive` entry (leaving `bodyclose` or other
valid linters intact); this aligns the file with `linters.default: none` and
avoids a no-op exclusion for a linter that is not enabled.
In `@internal/controller/organization/rbac.go`:
- Around line 176-180: Add a new constant roleKind = "Role" (to mirror existing
clusterRoleKind) and replace the string literal "Role" wherever used (for
example in the RoleRef assignment in roleBinding.RoleRef and the other
occurrence referenced) with the new roleKind constant; update references in
functions and structs that currently use the literal so all role kinds use the
roleKind symbol for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1494befc-ec3b-4b2d-bb73-051175bd4dac
📒 Files selected for processing (30)
.golangci.yamlcmd/service-proxy/instrumentation.goe2e/shared/utils.gogo.modinternal/clientutil/patch.gointernal/common/url.gointernal/controller/catalog/artifact.gointernal/controller/catalog/catalog_controller.gointernal/controller/cluster/bootstrap_controller.gointernal/controller/cluster/cluster_controller.gointernal/controller/organization/organization_controller_test.gointernal/controller/organization/rbac.gointernal/controller/organization/service_proxy.gointernal/controller/plugin/plugin_controller.gointernal/controller/plugin/pluginpreset_controller.gointernal/controller/plugindefinition/cluster_plugindefinition_controller.gointernal/controller/plugindefinition/plugindefinition_controller.gointernal/dex/connector.gointernal/features/features_test.gointernal/flux/helm_release_builder.gointernal/flux/utils.gointernal/local/klient/kind.gointernal/local/setup/manager.gointernal/local/setup/manifest.gointernal/local/setup/webhook.gointernal/local/utils/utils.gointernal/test/util.gopkg/lifecycle/context.gopkg/lifecycle/propagation.gopkg/lifecycle/propagation_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/flux/helm_release_builder.go
- internal/controller/catalog/catalog_controller.go
- internal/common/url.go
- internal/local/utils/utils.go
- internal/test/util.go
- internal/flux/utils.go
- internal/local/setup/manager.go
- cmd/service-proxy/instrumentation.go
- internal/local/setup/webhook.go
- internal/clientutil/patch.go
- go.mod
- internal/controller/cluster/cluster_controller.go
- internal/local/setup/manifest.go
- internal/controller/catalog/artifact.go
- pkg/lifecycle/propagation_test.go
- internal/controller/plugin/pluginpreset_controller.go
- internal/dex/connector.go
IvoGoman
force-pushed
the
refactor/golangci-lint
branch
from
783c948 to
f8ef79d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
internal/local/setup/webhook.go (2)
116-121:⚠️ Potential issue | 🟠 MajorGuard the annotations type assertion to prevent panics.
The direct assertion
annAny.(map[string]any)on line 117 will panic if annotations exist but are not a map type (e.g., malformed YAML). Use a safe type assertion pattern.Proposed fix
if annAny, found := metadata["annotations"]; found { - a := annAny.(map[string]any) - annotations = a + if a, ok := annAny.(map[string]any); ok { + annotations = a + } else { + annotations = make(map[string]any) + } } else {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/local/setup/webhook.go` around lines 116 - 121, The code currently does an unsafe type assertion annAny.(map[string]any) which can panic if metadata["annotations"] exists but is not a map; update the block that sets annotations (using the metadata variable and annAny) to use the comma-ok form (e.g., a, ok := annAny.(map[string]any)) and only assign to annotations when ok is true, otherwise initialize annotations to an empty map[string]any and optionally log or ignore the malformed value; ensure the safe-assertion replaces the direct assertion in the webhook.go function where annotations is set.
386-391:⚠️ Potential issue | 🟠 MajorUse safe type assertion for net.Addr to prevent panics.
The direct assertion
addr.(*net.IPNet)will panic if the OS returns a non-*net.IPNetimplementation ofnet.Addr. WhileInterface.Addrs()typically returns*net.IPNeton Linux, the interface contract doesn't guarantee this.Proposed fix
for _, addr := range addresses { - if ipv4 := addr.(*net.IPNet).IP.To4(); ipv4 != nil { + ipNet, ok := addr.(*net.IPNet) + if !ok || ipNet.IP == nil { + continue + } + if ipv4 := ipNet.IP.To4(); ipv4 != nil { utils.Logf("found IP address for docker0 interface: %s", ipv4.String()) return ipv4.String() } },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/local/setup/webhook.go` around lines 386 - 391, The loop over addresses uses a direct type assertion addr.(*net.IPNet) which can panic; change it to a safe type check (e.g., use a type assertion with the ok pattern or a type switch) to handle non-*net.IPNet implementations of net.Addr (and optionally *net.IPAddr), skip non-matching types, then call .IP.To4() on the validated IPNet/IPAddr and return the string when not nil; update the loop around addresses in webhook.go accordingly (look for the for _, addr := range addresses loop).e2e/shared/utils.go (1)
14-17:⚠️ Potential issue | 🟠 MajorGuard against panics from empty args or non-string first argument.
The code will panic at runtime if
Logis called with no arguments (index out of bounds) or ifargs[0]is not a string (type assertion failure). Removing//nolint:errcheckexposes this issue to linters but doesn't fix the underlying problem.Proposed fix
+import "fmt" + func Log(args ...any) { - args[0] = "===== 🤖 " + args[0].(string) - klog.InfoDepth(1, args...) + if len(args) == 0 { + klog.InfoDepth(1, "===== 🤖") + return + } + msgArgs := append([]any(nil), args...) + msgArgs[0] = "===== 🤖 " + fmt.Sprint(msgArgs[0]) + klog.InfoDepth(1, msgArgs...) },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/shared/utils.go` around lines 14 - 17, The Log function can panic when called with zero args or if args[0] is not a string; fix it in Log by first checking len(args)==0 and if so call klog.InfoDepth(1, "===== 🤖") or set args = []any{"===== 🤖"}; otherwise coerce the first element to string safely (e.g., use fmt.Sprint(args[0]) or similar) instead of a direct type assertion, then prefix it with "===== 🤖 " and call klog.InfoDepth(1, args...); update imports if you add fmt.internal/local/setup/manifest.go (1)
131-138:⚠️ Potential issue | 🟠 MajorUse defensive type assertions to prevent panics on malformed manifests.
The direct type assertions on lines 132-133 can panic if the manifest structure is unexpected (e.g.,
metadataexists but is not amap[string]any, ornameis missing/not a string). The nil check on line 131 doesn't guard against type mismatches.Proposed fix
if deployment["metadata"] != nil { - metadata := deployment["metadata"].(map[string]any) - name := metadata["name"].(string) + metadata, ok := deployment["metadata"].(map[string]any) + if !ok { + return fmt.Errorf("deployment metadata has unexpected type: %T", deployment["metadata"]) + } + name, ok := metadata["name"].(string) + if !ok || name == "" { + return fmt.Errorf("deployment metadata.name is missing or invalid") + } err = m.waitUntilDeploymentReady(ctx, clusterName, name, namespace),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/local/setup/manifest.go` around lines 131 - 138, The code does unsafe type assertions on deployment["metadata"] and metadata["name"] which can panic; replace them with defensive checks using the "ok" idiom: verify deployment["metadata"] is present and is a map[string]any (e.g., metadata, ok := deployment["metadata"].(map[string]any)), then verify metadata["name"] is a string (name, ok := metadata["name"].(string)), and handle the false cases by returning a clear error (or skipping) before calling m.waitUntilDeploymentReady(ctx, clusterName, name, namespace); ensure these checks surround the existing call to m.waitUntilDeploymentReady to avoid panics on malformed manifests.internal/local/utils/utils.go (1)
34-37:⚠️ Potential issue | 🟠 MajorGuard against panics from empty args or non-string first argument.
Same issue as
e2e/shared/utils.go: the code will panic if called with no arguments or ifargs[0]is not a string. Removing//nolint:errcheckdoesn't address the runtime safety concern.Proposed fix
func Log(args ...any) { - args[0] = "===== 🤖 " + args[0].(string) - klog.InfoDepth(1, args...) + if len(args) == 0 { + klog.InfoDepth(1, "===== 🤖") + return + } + msgArgs := append([]any(nil), args...) + msgArgs[0] = "===== 🤖 " + fmt.Sprint(msgArgs[0]) + klog.InfoDepth(1, msgArgs...) },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/local/utils/utils.go` around lines 34 - 37, The Log function can panic when called with zero arguments or when args[0] is not a string; fix Log by guarding early: if len(args) == 0 call klog.InfoDepth(1, "===== 🤖") and return; otherwise safely attempt a type assertion on args[0] (s, ok := args[0].(string)) and if ok set args[0] = "===== 🤖 " + s, else replace args[0] with the prefixed string produced via fmt.Sprint(args[0]) (or prepend a new prefixed string and leave the rest of args intact), then call klog.InfoDepth(1, args...); update imports to include fmt if you use fmt.Sprint.
🧹 Nitpick comments (1)
.golangci.yaml (1)
74-80: Enable type-assertion checks and scope exceptions narrowly.Setting
check-type-assertions: falserepo-wide can hide silent fallback bugs (comma-ok assertions returning zero values). The codebase has concrete instances incmd/service-proxy/instrumentation.go(lines 18, 23, 28) where type assertions are discarded—these should either be fixed to handle the assertion result properly or explicitly excluded if truly safe.Proposed config adjustment
errcheck: - check-type-assertions: false + check-type-assertions: true # Report about assignment of errors to blank identifier. check-blank: true - # Do not report about not checking of errors in type assertions. - # This is not as dangerous as skipping error values because an unchecked type assertion just immediately panics. - # We disable this because it makes a ton of useless noise esp. in test code. exclude-functions: - encoding/json.Marshal - (net/http.ResponseWriter).Write - (*github.com/spf13/cobra.Command).Help🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yaml around lines 74 - 80, Enable check-type-assertions in the linter and fix the three unsafe discarded type assertions in cmd/service-proxy/instrumentation.go by updating the assertions to use the comma-ok form (e.g., v, ok := x.(T)) and handle the !ok case or return an error, or if a cast is provably safe, add a narrowly scoped exception in exclude-functions for the specific function names where those assertions occur; update .golangci.yaml to set check-type-assertions: true and only add explicit excludes for the exact function identifiers instead of disabling the check repo-wide.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/shared/utils.go`:
- Around line 14-17: The Log function can panic when called with zero args or if
args[0] is not a string; fix it in Log by first checking len(args)==0 and if so
call klog.InfoDepth(1, "===== 🤖") or set args = []any{"===== 🤖"}; otherwise
coerce the first element to string safely (e.g., use fmt.Sprint(args[0]) or
similar) instead of a direct type assertion, then prefix it with "===== 🤖 " and
call klog.InfoDepth(1, args...); update imports if you add fmt.
In `@internal/local/setup/manifest.go`:
- Around line 131-138: The code does unsafe type assertions on
deployment["metadata"] and metadata["name"] which can panic; replace them with
defensive checks using the "ok" idiom: verify deployment["metadata"] is present
and is a map[string]any (e.g., metadata, ok :=
deployment["metadata"].(map[string]any)), then verify metadata["name"] is a
string (name, ok := metadata["name"].(string)), and handle the false cases by
returning a clear error (or skipping) before calling
m.waitUntilDeploymentReady(ctx, clusterName, name, namespace); ensure these
checks surround the existing call to m.waitUntilDeploymentReady to avoid panics
on malformed manifests.
In `@internal/local/setup/webhook.go`:
- Around line 116-121: The code currently does an unsafe type assertion
annAny.(map[string]any) which can panic if metadata["annotations"] exists but is
not a map; update the block that sets annotations (using the metadata variable
and annAny) to use the comma-ok form (e.g., a, ok := annAny.(map[string]any))
and only assign to annotations when ok is true, otherwise initialize annotations
to an empty map[string]any and optionally log or ignore the malformed value;
ensure the safe-assertion replaces the direct assertion in the webhook.go
function where annotations is set.
- Around line 386-391: The loop over addresses uses a direct type assertion
addr.(*net.IPNet) which can panic; change it to a safe type check (e.g., use a
type assertion with the ok pattern or a type switch) to handle non-*net.IPNet
implementations of net.Addr (and optionally *net.IPAddr), skip non-matching
types, then call .IP.To4() on the validated IPNet/IPAddr and return the string
when not nil; update the loop around addresses in webhook.go accordingly (look
for the for _, addr := range addresses loop).
In `@internal/local/utils/utils.go`:
- Around line 34-37: The Log function can panic when called with zero arguments
or when args[0] is not a string; fix Log by guarding early: if len(args) == 0
call klog.InfoDepth(1, "===== 🤖") and return; otherwise safely attempt a type
assertion on args[0] (s, ok := args[0].(string)) and if ok set args[0] = "=====
🤖 " + s, else replace args[0] with the prefixed string produced via
fmt.Sprint(args[0]) (or prepend a new prefixed string and leave the rest of args
intact), then call klog.InfoDepth(1, args...); update imports to include fmt if
you use fmt.Sprint.
---
Nitpick comments:
In @.golangci.yaml:
- Around line 74-80: Enable check-type-assertions in the linter and fix the
three unsafe discarded type assertions in cmd/service-proxy/instrumentation.go
by updating the assertions to use the comma-ok form (e.g., v, ok := x.(T)) and
handle the !ok case or return an error, or if a cast is provably safe, add a
narrowly scoped exception in exclude-functions for the specific function names
where those assertions occur; update .golangci.yaml to set
check-type-assertions: true and only add explicit excludes for the exact
function identifiers instead of disabling the check repo-wide.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7ad60e9-b3fa-4033-b839-09b2f1e02b6b
📒 Files selected for processing (30)
.golangci.yamlcmd/service-proxy/instrumentation.goe2e/shared/utils.gogo.modinternal/clientutil/patch.gointernal/common/url.gointernal/controller/catalog/artifact.gointernal/controller/catalog/catalog_controller.gointernal/controller/cluster/bootstrap_controller.gointernal/controller/cluster/cluster_controller.gointernal/controller/organization/organization_controller_test.gointernal/controller/organization/rbac.gointernal/controller/organization/service_proxy.gointernal/controller/plugin/plugin_controller.gointernal/controller/plugin/pluginpreset_controller.gointernal/controller/plugindefinition/cluster_plugindefinition_controller.gointernal/controller/plugindefinition/plugindefinition_controller.gointernal/dex/connector.gointernal/features/features_test.gointernal/flux/helm_release_builder.gointernal/flux/utils.gointernal/local/klient/kind.gointernal/local/setup/manager.gointernal/local/setup/manifest.gointernal/local/setup/webhook.gointernal/local/utils/utils.gointernal/test/util.gopkg/lifecycle/context.gopkg/lifecycle/propagation.gopkg/lifecycle/propagation_test.go
✅ Files skipped from review due to trivial changes (4)
- internal/clientutil/patch.go
- internal/controller/plugindefinition/plugindefinition_controller.go
- internal/controller/organization/organization_controller_test.go
- internal/controller/cluster/bootstrap_controller.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/local/setup/manager.go
- internal/features/features_test.go
- internal/controller/cluster/cluster_controller.go
- internal/test/util.go
- internal/common/url.go
- internal/controller/plugin/pluginpreset_controller.go
- pkg/lifecycle/propagation_test.go
- internal/flux/helm_release_builder.go
- internal/controller/plugindefinition/cluster_plugindefinition_controller.go
- pkg/lifecycle/context.go
- internal/flux/utils.go
- internal/controller/organization/rbac.go
IvoGoman
force-pushed
the
refactor/golangci-lint
branch
from
f8ef79d to
1333051
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/local/klient/kind.go (1)
68-71: Good use of stdlibslices.Contains.The migration from a manual loop to
slices.Containsis a clean improvement. Consider simplifying further by returning directly:♻️ Optional simplification
utils.Logf("checking if cluster %s exists...", clusterName) - if slices.Contains(clusters, clusterName) { - return true, nil - } - return false, nil + return slices.Contains(clusters, clusterName), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/local/klient/kind.go` around lines 68 - 71, Replace the verbose if/return block checking slices.Contains with a single return that directly returns the boolean result and nil error; specifically, in the function using clusters and clusterName (currently calling slices.Contains), change the two-branch return to: return slices.Contains(clusters, clusterName), nil so the function is simplified.internal/controller/catalog/catalog_controller.go (1)
85-85: Unchecked type assertions rely on lifecycle framework guarantees.The
nolint:errcheckremoval exposes unchecked type assertions. While thelifecycle.Reconcilecall (line 76) passes&greenhousev1alpha1.Catalog{}as the template object, which should ensure type consistency in callbacks, a defensive check would prevent panics if the lifecycle contract is ever violated.This is a low-risk pattern since the lifecycle framework controls the object type, but worth noting for consistency with defensive coding practices.
Also applies to: 216-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/catalog/catalog_controller.go` at line 85, Replace the unchecked type assertion "catalog := obj.(*greenhousev1alpha1.Catalog)" with a safe type check using the comma-ok idiom (e.g., catalog, ok := obj.(*greenhousev1alpha1.Catalog)); if ok is false, log an error (using the controller's logger or reconcile request context) and return an appropriate error/result to avoid panic. Apply the same defensive check for the other unchecked assertion referenced in this file (the second occurrence at the same pattern) so lifecycle.Reconcile callbacks gracefully handle unexpected types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/catalog/catalog_controller.go`:
- Line 85: Replace the unchecked type assertion "catalog :=
obj.(*greenhousev1alpha1.Catalog)" with a safe type check using the comma-ok
idiom (e.g., catalog, ok := obj.(*greenhousev1alpha1.Catalog)); if ok is false,
log an error (using the controller's logger or reconcile request context) and
return an appropriate error/result to avoid panic. Apply the same defensive
check for the other unchecked assertion referenced in this file (the second
occurrence at the same pattern) so lifecycle.Reconcile callbacks gracefully
handle unexpected types.
In `@internal/local/klient/kind.go`:
- Around line 68-71: Replace the verbose if/return block checking
slices.Contains with a single return that directly returns the boolean result
and nil error; specifically, in the function using clusters and clusterName
(currently calling slices.Contains), change the two-branch return to: return
slices.Contains(clusters, clusterName), nil so the function is simplified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b83890b-d6e7-4d33-965e-9b872e01af22
📒 Files selected for processing (30)
.golangci.yamlcmd/service-proxy/instrumentation.goe2e/shared/utils.gogo.modinternal/clientutil/patch.gointernal/common/url.gointernal/controller/catalog/artifact.gointernal/controller/catalog/catalog_controller.gointernal/controller/cluster/bootstrap_controller.gointernal/controller/cluster/cluster_controller.gointernal/controller/organization/organization_controller_test.gointernal/controller/organization/rbac.gointernal/controller/organization/service_proxy.gointernal/controller/plugin/plugin_controller.gointernal/controller/plugin/pluginpreset_controller.gointernal/controller/plugindefinition/cluster_plugindefinition_controller.gointernal/controller/plugindefinition/plugindefinition_controller.gointernal/dex/connector.gointernal/features/features_test.gointernal/flux/helm_release_builder.gointernal/flux/utils.gointernal/local/klient/kind.gointernal/local/setup/manager.gointernal/local/setup/manifest.gointernal/local/setup/webhook.gointernal/local/utils/utils.gointernal/test/util.gopkg/lifecycle/context.gopkg/lifecycle/propagation.gopkg/lifecycle/propagation_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/local/setup/webhook.go
🚧 Files skipped from review as they are similar to previous changes (14)
- cmd/service-proxy/instrumentation.go
- internal/flux/utils.go
- internal/features/features_test.go
- internal/clientutil/patch.go
- internal/local/setup/manifest.go
- internal/controller/organization/organization_controller_test.go
- internal/controller/plugindefinition/plugindefinition_controller.go
- internal/controller/plugin/plugin_controller.go
- pkg/lifecycle/propagation_test.go
- internal/controller/organization/rbac.go
- pkg/lifecycle/context.go
- internal/controller/organization/service_proxy.go
- internal/controller/cluster/cluster_controller.go
- e2e/shared/utils.go
IvoGoman
force-pushed
the
refactor/golangci-lint
branch
from
1333051 to
40aa72c
Compare
Zaggy21
approved these changes
Mar 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added to documentation?
Checklist
Summary by CodeRabbit
Chores
Refactor