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

Add golangci-lint CI action, fix gosimple, govet + unused lint errors #1127

Merged
merged 3 commits into from
May 31, 2022

Conversation

timvaillancourt
Copy link
Collaborator

@timvaillancourt timvaillancourt commented May 29, 2022

Description

This PR adds a golangci-lint config and GitHub Action in order to lint the gh-ost code

Currently nothing is validating the cleanliness/best-practices of our Golang code

Initially only these linters are enabled:

  1. gosimple

    Linter for Go source code that specializes in simplifying a code

  2. govet

    Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string

  3. unused

    Checks Go code for unused constants, variables, functions and types

And I plan to create follow-up PRs to enable the remaining linters

Introducing these 3 x linters triggered these complaints that are resolved by this PR:
Screen Shot 2022-05-29 at 04 00 15
Linter errors raised/resolved:

  1. func REDACTED is unused (unused)
  2. S1025: should use String() instead of fmt.Sprintf (gosimple)
  3. S1039: unnecessary use of fmt.Sprintf (gosimple)
  4. S1019: should use make([]string, len(uniqueKeyColumnNames)) instead (gosimple)
  5. S1023: redundant return statement (gosimple)
  6. S1007: should use raw string (...) with regexp.MustCompile to avoid having to escape twice (gosimple)
  7. S1011: should replace loop with env = append(env, extraVariables...) (gosimple)
  8. S1038: should use fmt.Fprintf instead of fmt.Fprintln(fmt.Sprintf(...)) (but don't forget the newline) (gosimple)
  9. composites: REDACTED composite literal uses unkeyed fields (govet)
  10. unreachable: unreachable code (govet)
    • Removed unused executeAndThrottleOnError func from go/logic/migrator.go
  • contributed code is using same conventions as original code
  • script/cibuild returns with no formatting errors, build errors or unit test errors.

@timvaillancourt timvaillancourt self-assigned this May 29, 2022
@timvaillancourt timvaillancourt changed the title Add golangci-lint, fix gosimple, govet + unused linter errors Add golangci-lint CI action, fix gosimple, govet + unused lint errors May 29, 2022
Copy link
Member

@rashiq rashiq left a comment

Choose a reason for hiding this comment

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

❤️ 🚀

@timvaillancourt
Copy link
Collaborator Author

timvaillancourt commented May 30, 2022

Merging as this doesn't change logic and unit tests are passing 👍

EDIT: never mind, I don't have access to force-merge a PR with incomplete required jobs

@timvaillancourt
Copy link
Collaborator Author

timvaillancourt commented May 30, 2022

@rashiq / @dm-2: there are still 2 x CI jobs that are required although we removed them in #1097 and #1101:

  1. build (mysql-5.5.62)
  2. build (mysql-5.6.43)

Could you help remove those 2 x stale jobs as required in the "branch protection rules" (under "Settings" for this repo) for master? 🙇

@dm-2
Copy link
Contributor

dm-2 commented May 31, 2022

@rashiq / @dm-2: there are still 2 x CI jobs that are required although we removed them in #1097 and #1101:

  1. build (mysql-5.5.62)
  2. build (mysql-5.6.43)

Could you help remove those 2 x stale jobs as required in the "branch protection rules" (under "Settings" for this repo) for master? 🙇

Done, mystery solved about where those jobs were configured - thanks! I've run the CI build now so you should be all good to merge once that's done 👍

@timvaillancourt
Copy link
Collaborator Author

Done, mystery solved about where those jobs were configured - thanks! I've run the CI build now so you should be all good to merge once that's done 👍

@dm-2 👋 and thanks!

@timvaillancourt timvaillancourt merged commit ed46138 into github:master May 31, 2022
@timvaillancourt timvaillancourt deleted the golangci-lint-part-1 branch May 31, 2022 19:23
@timvaillancourt timvaillancourt added this to the v1.1.5 milestone Jun 2, 2022
dm-2 pushed a commit that referenced this pull request Jul 7, 2022
…t errors (#1127)

* Add `golangci-lint`, fix `gosimple`, `govet` and `unused` linter complaints

* Go 1.16

* Update copyright dates
@dm-2 dm-2 mentioned this pull request Jul 7, 2022
dm-2 pushed a commit that referenced this pull request Jul 7, 2022
…t errors (#1127)

* Add `golangci-lint`, fix `gosimple`, `govet` and `unused` linter complaints

* Go 1.16

* Update copyright dates
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.

3 participants