Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: upgrade golangci linter to 1.57.2 #9279

Merged
merged 3 commits into from
Jun 6, 2024
Merged

Conversation

carolinaecalderon
Copy link
Contributor

@carolinaecalderon carolinaecalderon commented May 1, 2024

Ticket

RM-169

Description

Upgrade the golangci linter to version 1.57.2.

Includes the following changes (not exhaustive):

  • Replace a switch with only one case with an if-then.
  • In some cases, fmt.Sprintf can be replaced with strconv.Itoa.
  • Variables should be renamed to avoid redefinition of any built-in functions. (i.e., a function named close should be renamed to closeDB).
  • In tests with require statments, require that the expected value come before the actual.
  • In workflows with if cond{foo()} return bar, replace this with if !cond {return bar} foo().
  • Remove unused function parameters & return values.
  • Remove useless breaks in case clauses.
  • Variables named *Ids should be renamed to *IDs.
  • Lowercase all unexported variables & functions.
  • For functions that return several variables of the same type, name the variables.

Additionally, whenever I came across cases of errors.New(...) in files I was already changing, I replaced these with fmt.Errorf(...) -- as this is best practice, in our pursuit to remove the errors pkg from our code base. I didn't go out of my way to replace all instances of errors.New as I wanted to limit the scope of this ticket.

Allows the following new linters:

  • gosimple
  • gosec: inspects source code for security problems; i.e., implicit memory aliasing in for loop.
  • depguard: check if package imports are in a list of acceptable packages, as specified in the .golangci.yml.
  • testifylint: checks usage of github.com/stretchr/testify; enable blank-import bool-compare, compares, empty, error-is-as, error-nil, expected-actual, float-compare, len, negative-positive, nil-compare, require-error, suite-dont-use-pkg, suite-extra-assert-call, suite-thelper, useless-asser. Disable go-require. (see source code for more context).
  • revive: fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint.

Disable the following linters: tagalign, protogetter, inamedparam, nakedret & a subset of the revive sub-linters. See the yaml file for more context.

Clean up the golang yaml (master/.golangci.yml), updating the code & removing deprecated mentions.

Test Plan

N/A -- pass current circleCI checks.

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@cla-bot cla-bot bot added the cla-signed label May 1, 2024
Copy link

netlify bot commented May 1, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit d83a2e9
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6661d4d725b4620008bd8299

Copy link

codecov bot commented May 1, 2024

Codecov Report

Attention: Patch coverage is 23.07692% with 200 lines in your changes missing coverage. Please review.

Project coverage is 49.00%. Comparing base (934aeb6) to head (d83a2e9).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9279      +/-   ##
==========================================
- Coverage   49.00%   49.00%   -0.01%     
==========================================
  Files        1234     1234              
  Lines      159693   159652      -41     
  Branches     2781     2781              
==========================================
- Hits        78259    78235      -24     
+ Misses      81259    81242      -17     
  Partials      175      175              
Flag Coverage Δ
backend 43.78% <23.07%> (-0.01%) ⬇️
harness 64.04% <ø> (-0.01%) ⬇️
web 44.12% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
agent/internal/container/container.go 85.60% <100.00%> (-0.06%) ⬇️
master/internal/api/apiutils/mask.go 100.00% <ø> (ø)
master/internal/api/query.go 0.00% <ø> (ø)
master/internal/api_auth.go 48.59% <100.00%> (ø)
master/internal/api_workspace.go 62.78% <100.00%> (ø)
...ernal/config/dispatcher_resource_manager_config.go 35.29% <ø> (-0.94%) ⬇️
master/internal/config/resource_manager_config.go 65.26% <ø> (-0.37%) ⬇️
master/internal/core.go 6.47% <ø> (ø)
master/internal/db/postgres_trial.go 63.20% <ø> (+0.09%) ⬆️
master/internal/db/postgres_trial_metrics.go 91.53% <100.00%> (ø)
... and 69 more

... and 3 files with indirect coverage changes

@carolinaecalderon carolinaecalderon changed the title Carolinac/rm 169 chore: upgrade golangci linter to 1.57.2 May 28, 2024
master/.golangci.yml Outdated Show resolved Hide resolved
agent/internal/agent.go Outdated Show resolved Hide resolved
master/.golangci.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@kkunapuli kkunapuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for making all these changes - I'm really excited about it; I think it cleans up a lot of nits in the code making it more idiomatic and easier to debug/reason about.

agent/pkg/singularity/singularity.go Show resolved Hide resolved
master/internal/plugin/oidc/service.go Show resolved Hide resolved
Copy link
Contributor

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked the config changes, and a random sample of the files. Thank you!

@carolinaecalderon carolinaecalderon merged commit 84299a6 into main Jun 6, 2024
98 of 118 checks passed
@carolinaecalderon carolinaecalderon deleted the carolinac/rm-169 branch June 6, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants