Fix gocritic lint warnings#37
Conversation
- Rewrite if-else chains to switch statements in tests and handlers - Hoist constant regexp to package level with MustCompile - Use += assignment operator - Replace log.Fatal with log.Println + return so deferred cleanup runs
cf4a914 to
6636bff
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses gocritic warnings flagged by golangci-lint across the proxy codebase, primarily by refactoring control flow, reducing repeated work, and adjusting error-exit behavior.
Changes:
- Replaced several
if/elsechains withswitchstatements. - Precompiled a Python index URL regexp to avoid compiling it per request.
- Replaced some
log.Fatal(...)exits withlog.Println(...)+returnto allow deferred cleanup to run.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| main.go | Replaces log.Fatal with log.Println + return in several failure paths. |
| internal/oidc/oidc_credential.go | Refactors OIDC parameter selection from if/else to switch. |
| internal/logging/logging.go | Uses += for string concatenation in cached-response logging. |
| internal/handlers/python_index.go | Moves regexp compilation to a package-level regexp.MustCompile. |
| internal/handlers/nuget_feed.go | Refactors response-type branching from if/else to switch. |
| internal/handlers/github_api_test.go | Converts an if/else chain to a switch in a table-driven test. |
| internal/handlers/git_server_test.go | Converts an if/else chain to a switch in a table-driven test. |
Comments suppressed due to low confidence (1)
main.go:100
- With
log.Println(err); returnonListenAndServefailure,proxy.Close()(which writes cache/metrics) is skipped. If the goal is to avoidlog.Fatalwhile still running defers, considerdeferingproxy.Close()right afternewProxy(...)so it runs on all exit paths.
log.Printf("Listening (%s)", *addr)
if err := server.ListenAndServe(); err != http.ErrServerClosed {
log.Println(err)
return
}
if err := proxy.Close(); err != nil {
log.Println(err)
return
| urls = handleV3Response(bodyReader, url) | ||
| } else { | ||
| default: | ||
| logging.RequestLogf(nil, "unknown API response: %s...", bodyString[:10]) |
There was a problem hiding this comment.
bodyString[:10] will panic when the response body is shorter than 10 bytes (including empty/whitespace-only bodies). Guard the slice with a length check (or log the whole string capped safely) before slicing.
| logging.RequestLogf(nil, "unknown API response: %s...", bodyString[:10]) | |
| preview := bodyString | |
| if len(preview) > 10 { | |
| preview = preview[:10] | |
| } | |
| logging.RequestLogf(nil, "unknown API response: %s...", preview) |
| awsRegion := cred.GetString("aws-region") | ||
| accountID := cred.GetString("account-id") | ||
| roleName := cred.GetString("role-name") | ||
| domain := cred.GetString("domain") | ||
| domainOwner := cred.GetString("domain-owner") | ||
|
|
||
| if tenantID != "" && clientID != "" { | ||
| switch { | ||
| case tenantID != "" && clientID != "": | ||
| parameters = &AzureOIDCParameters{ | ||
| TenantID: tenantID, | ||
| ClientID: clientID, | ||
| } | ||
| } else if jfrogOidcProviderName != "" && feedUrl != "" { | ||
| case jfrogOidcProviderName != "" && feedUrl != "": | ||
| // jfrog domain is extracted from feed url | ||
| jfrogUrlParsed, err := url.Parse(feedUrl) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid jfrog url: %w", err) | ||
| } | ||
| parameters = &JFrogOIDCParameters{ | ||
| // required | ||
| JFrogURL: fmt.Sprintf("%s://%s", jfrogUrlParsed.Scheme, jfrogUrlParsed.Host), | ||
| ProviderName: jfrogOidcProviderName, | ||
| // optional | ||
| Audience: cred.GetString("audience"), | ||
| IdentityMappingName: cred.GetString("identity-mapping-name"), | ||
| } | ||
| } else if awsRegion != "" && accountID != "" && roleName != "" && domain != "" && domainOwner != "" { | ||
| case awsRegion != "" && accountID != "" && roleName != "" && domain != "" && domainOwner != "": | ||
| audience := cred.GetString("audience") |
There was a problem hiding this comment.
rolleName is defined from cred.GetString("role-name"), but the AWS case still references roleName, which is undefined. This will fail to compile and also drops the role-name value. Rename rolleName back to roleName (or update the switch case and struct assignment to use the same identifier).
| cfg, err := config.Parse(*configPath) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| log.Println(err) | ||
| return | ||
| } | ||
|
|
||
| sentry, err := setupSentry() | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| log.Println(err) | ||
| return | ||
| } |
There was a problem hiding this comment.
These early returns from main() log the error but exit with status code 0. For a CLI/service this can make startup failures look like success to the caller/supervisor. Consider returning a non-zero exit code (e.g., track an exitCode and defer os.Exit(exitCode) so deferred file.Close() still runs).
This issue also appears on line 92 of the same file.
Fixes the 8 gocritic warnings reported by golangci-lint.
What changed:
ifElseChain: Converted 4 if-else chains to switch statements (ingit_server_test.go,github_api_test.go,nuget_feed.go,oidc_credential.go)regexpMust: Moved a constant regexp pattern inpython_index.goto a package-levelregexp.MustCompileso it's compiled once instead of on every requestassignOp: Used+=instead ofx = x + yinlogging.goexitAfterDefer: Replacedlog.Fatalcalls inmain.gowithlog.Println+returnso deferredfile.Close()actually runs on early exit