fix(lint): resolve unparam findings - #892
Merged
Merged
Conversation
✅ Deploy Preview for devsydev canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThe change simplifies Go function contracts by removing unused values and always-nil errors. Docker E2E helpers now use fixed provider setup and return required container data. Kubernetes, platform, telemetry, workspace, and copy callers use the updated contracts. ChangesDocker E2E helper contracts
Runtime return contract simplification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
✅ Deploy Preview for images-devsy-sh canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/tests/up-docker-compose/helper.go`:
- Around line 113-116: Update the error branch in verifyWorkspaceMount so a
successful findComposeContainer call with no IDs is handled as an error rather
than returning nil, nil; remove the len(ids) == 0 condition and allow
inspectContainer to produce its existing error, or return an explicit error
before dereferencing the container detail.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f63904b0-a707-40fc-976c-35039e1c86f4
📒 Files selected for processing (14)
e2e/tests/up-docker-compose/build.goe2e/tests/up-docker-compose/config.goe2e/tests/up-docker-compose/helper.goe2e/tests/up-docker-compose/up_docker_compose.goe2e/tests/up-features/helper.gopkg/copy/copy.gopkg/daemon/platform/daemon.gopkg/daemon/platform/local_server.gopkg/driver/kubernetes/init_container.gopkg/driver/kubernetes/registry.gopkg/driver/kubernetes/run.gopkg/platform/kubeconfig.gopkg/telemetry/collect.gopkg/workspace/list.go
skevetter
marked this pull request as draft
August 7, 2026 01:18
Removed unused return values and constant-valued parameters across 9
findings, updating every call site:
- pkg/copy/copy.go: parseUserSpec drops its unused group return.
- pkg/daemon/platform/local_server.go: newLocalServer and watchPlatform
drop their always-nil error returns (unexported, single callers);
ListenAndServe/Close synchronization via stopChan is unaffected.
- pkg/driver/kubernetes/init_container.go: getInitContainers drops its
always-nil error return.
- pkg/driver/kubernetes/registry.go: validateIndexName drops its
always-nil error return; propagated into newIndexInfo (its only
caller), which had the same always-nil pattern once validateIndexName's
signature simplified -- not in the original 11 but a direct consequence
of this fix, so simplified alongside to keep the lint run clean.
- pkg/platform/kubeconfig.go: newKubeConfig drops its always-true
insecure param (InsecureSkipTLSVerify is always true across its 4
call sites; no test coverage depends on making it configurable).
- pkg/telemetry/collect.go: newCLICollector drops its always-nil error
return (analytics.NewClient() cannot fail).
- pkg/workspace/list.go: listProWorkspaces drops its always-nil error
return; propagated into reconcileProWorkspaces (its only caller, whose
own error return became always-nil as a result) for the same reason
as newIndexInfo above.
- e2e/tests/up-docker-compose/helper.go: getAppContainer drops its
unused ids return; setupDockerProvider drops its always-"docker"
dockerPath param (all 5 call sites in this package use "docker").
Suppressed rather than fixed:
- e2e/tests/up-features/helper.go: setupDockerProvider's dockerPath
looks constant ("docker") from every non-Windows call site, but
wsl.go (a windows-only build-tagged file in the same package) calls
it with "podman". Verified with GOOS=windows go build. Added
//nolint:unparam noting the windows-only caller.
go build, go vet (aside from a pre-existing, unrelated ptytest.go vet
finding), and tests for every touched package all pass. Also verified
GOOS=windows go build ./e2e/... to confirm the up-features suppression.
- getAppContainer no longer returns (nil, nil) when the compose container lookup succeeds with zero IDs; it now falls through to inspectContainer, which already errors on an empty ID list. This fixes a latent nil-pointer panic in verifyWorkspaceMount and the container-detail assertions in config.go. - Update newIndexInfo's comment to reflect that it now returns a normalized registry name string, not an IndexInfo struct.
skevetter
force-pushed
the
lint/unparam-findings
branch
from
August 7, 2026 04:10
75cf1fc to
d00b4c3
Compare
skevetter
marked this pull request as ready for review
August 7, 2026 05:46
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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
1 participant
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.
Resolves all 11
unparamfindings by removing unused return values / constant-valued params and updating every call site.Fixed, real signature simplification:
pkg/copy/copy.go:parseUserSpecdrops its unused group return.pkg/daemon/platform/local_server.go:newLocalServer/watchPlatformdrop their always-nil error returns (unexported, single caller each).ListenAndServe's stopChan-based shutdown synchronization is unaffected — verified by reading the full select/close path.pkg/driver/kubernetes/init_container.go:getInitContainersdrops its always-nil error return.pkg/driver/kubernetes/registry.go:validateIndexNamedrops its always-nil error return, propagated intonewIndexInfo(its only caller — became always-nil as a direct consequence, not in the original 11 but needed to keep unparam at 0).pkg/platform/kubeconfig.go:newKubeConfigdropsinsecure— all 4 call sites passtrue.pkg/telemetry/collect.go:newCLICollectordrops its always-nil error return.pkg/workspace/list.go:listProWorkspacesdrops its always-nil error return, propagated intoreconcileProWorkspaces(its only caller, same reasoning asnewIndexInfo).e2e/tests/up-docker-compose/helper.go:getAppContainerdrops its unusedidsreturn;setupDockerProviderdropsdockerPath(always "docker" across all 5 call sites in this package).Suppressed, with reason:
e2e/tests/up-features/helper.go:setupDockerProvider'sdockerPathlooks constant from every non-Windows call site, butwsl.go— a windows-only (//go:build windows) file in the same package — calls it with "podman". Removing the param would break the Windows build (confirmed withGOOS=windows go build ./e2e/..., both before and after my change). Added//nolint:unparamnoting this.golangci-lint run --enable-only=unparam --max-same-issues=0 ./...now reports 0 issues.go build,go vet(aside from one pre-existing, unrelatedpkg/pty/ptytestfinding), and tests for every touched package all pass.Summary by CodeRabbit