fix(lint): resolve dupl findings - #891
Conversation
Extracted a shared dialAndExecute helper (cmd/pro/provider/exec.go) for the genuinely duplicated dial+execute body in ssh.go/stop.go, parameterized by sub-resource action and env flags. The remaining three pairs are cobra command factory boilerplate (NewCheckUpdateCmd/NewHealthCmd, NewNetcheckCmd/NewStatusCmd) that already have an established repo convention of //nolint:dupl with a 'structurally similar; intentional sibling factory' reason (see NewCreateCmd/NewUpdateCmd, NewHealthCmd) -- followed the same convention rather than introducing a generic cobra-wiring abstraction across unrelated command types. Extracted a setupParentChildGraph helper for the two near-identical resolver_test.go cases, keeping them as separate, individually runnable test functions since they assert distinct precedence scenarios. pkg/ts/workspace_server.go's pair is intentionally left as-is: it's the exact handler pair refactored in #890 (Director -> Rewrite migration), not yet merged. Deferred to a follow-up to avoid a conflicting refactor.
✅ Deploy Preview for images-devsy-sh canceled.
|
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 devsydev canceled.
|
Two self-inflicted findings from the earlier dupl fix: - pkg/options/resolver/resolver_test.go: moved the unexported setupParentChildGraph helper after all exported ResolverTestSuite test methods (funcorder wants unexported methods last on a struct). - cmd/pro/provider/exec.go: dialAndExecute had 7 params, over the repo's revive argument-limit of 4. Bundled configPath/action/envFlags/stdin/ stdout/stderr into a dialAndExecuteParams struct (ctx stays separate), matching the existing <funcName>Params convention used elsewhere in cmd/pro (loginParams, createClusterParams, waitForWorkspacePhaseParams). Updated both call sites in ssh.go and stop.go. That struct extraction reduced ssh.go/stop.go's Run bodies to a single parameterized call, which made the two files dupl-flag themselves as a whole (lines 1-51 in each) -- the same cobra-sibling-factory pattern as the check_update/health and netcheck/status pairs already suppressed elsewhere in this PR. Added matching //nolint:dupl directives (had to place them above the package clause, since dupl's clone range for these two files starts at line 1).
1 participant
Resolves 3 of 5
duplduplicate pairs (9 findings). One pair is deferred; workspace_server.go's pair is untouched by design.Extracted a shared helper (genuine duplication):
cmd/pro/provider/ssh.go/stop.go: bothRunmethods duplicated the full dial-and-execute body (client init, workspace lookup, dial, stream). ExtracteddialAndExecuteinto a newcmd/pro/provider/exec.go, parameterized by sub-resource action and env flags.Suppressed, following existing repo convention:
cmd/pro/check_update.goNewCheckUpdateCmd/cmd/pro/health.goNewHealthCmdcmd/pro/daemon/netcheck.goNewNetcheckCmd/cmd/pro/daemon/status.goNewStatusCmdBoth are cobra command-factory boilerplate. The repo already has this exact pattern suppressed elsewhere (
NewCreateCmd/NewUpdateCmdincmd/pro/workspace, andNewHealthCmditself already carried a//nolint:dupl—NewCheckUpdateCmdwas just missing its half). Added matching//nolint:dupl // structurally similar to X; intentional sibling factorycomments rather than inventing a generic cobra-wiring abstraction across unrelated command types.Test duplication:
pkg/options/resolver/resolver_test.go: extractedsetupParentChildGraphfor the two near-identical parent/child precedence tests, kept as separate test functions since they assert distinct scenarios.Deferred (not touched):
pkg/ts/workspace_server.go'sgitCredentialsHandler/dockerCredentialsHandlerpair — this is the exact handler pair refactored in fix(lint): resolve staticcheck SA1019 findings #890 (Director→Rewrite), not yet merged. Fixing here would conflict; follow-up after fix(lint): resolve staticcheck SA1019 findings #890 merges.golangci-lint run --enable-only=dupl --max-same-issues=0 ./...now reports exactly the 2 remaining workspace_server.go findings (the deferred pair), not zero.