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

gofmt issue fixes as per goreportcard (#1274) #1749

Merged
merged 2 commits into from
Nov 29, 2022

Conversation

Bhargav-InfraCloud
Copy link
Contributor

Signed-off-by: Bhargav Ravuri bhargav.ravuri@infracloud.io

Change Overview

  • Fix the go formatting issues reported by goreportcardare.
  • Add gofmt in golint.sh to warn for future commits.
  • No functional or test logic changes made. This is only for go formatting.

Pull request type

Please check the type of change your PR introduces:

  • 🌈 Refactoring (no functional changes, no api changes)

Issues

Test Plan

  • 💪 Manual
  1. Run make golint.
  2. Install goreportcard and run goreportcard-cli -v. It should print stats similar to:
Grade .......... A+ 100.0%
Files .............. 31757
Issues ................. 0
gofmt ............... 100%
go_vet .............. 100%
ineffassign ......... 100%
gocyclo ............. 100%
license ............. 100%
misspell ............ 100%

Note: % of gofmt should be higher than what it was before this commit.

Message for maintainers

  • golint is archived and so it was disabled from goreportcard in commit.
  • Also, goreportcard uses gometalinter which is archived. The replacement is golangci-lint which is being used by Kanister already. However, most of the common linter in it (like, gofmt, revive, etc.) are not enabled or skipped for packages. I'd recommend to have a .golangci.yml configured to make most out of it.

# 2. Build exempted files using build tags
# Note: Future build tags should be included.
echo "Running gofmt..."
golangci-lint run --timeout ${TIMEOUT} --disable-all --enable gofmt --build-tags integration
Copy link
Contributor

Choose a reason for hiding this comment

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

@Bhargav-InfraCloud If we add this, we don't need the previous call (Line no 25), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would require both, actually. The first one (line 25) runs 7 default linters (errcheck, gosimple, govet, ineffassign, staticcheck, typecheck & unused) with 4 enabled (maligned, whitespace, gocognit & unparam).

The second (line 32) only runs gofmt which is the core of this change.

Ideally, gofmt could've been added to the first. But in doing so, the extra conditions (skipping the directory and excluding ctx error) will get applied to this which is not wise. And also, gofmt without --build-tags integration would ignore 2 files dropping the % in report. Enable it in first directly and it would throw a lot of unexpected linter errors (not required as of now).

As I suggested in PR, using .golangci-yaml can solve this issue, where we can specify all the configs and run it only once. But that would be for future PR if agreed upon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Bhargav-InfraCloud could you please file an issue to switch to .golanci-yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #1761

Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @Bhargav-InfraCloud 🙏🏼
LGTM. Will let @PrasadG193 +2 this

Fix the go formatting issues reported by gojp/goreportcardare.
Add 'gofmt' in golint.sh to warn for future commits.

Signed-off-by: Bhargav Ravuri <bhargav.ravuri@infracloud.io>
@Bhargav-InfraCloud
Copy link
Contributor Author

@pavannd1 @PrasadG193 The pipeline probably was failing because of commit e956100 or b48647f that I merged from master into this branch without checking the github action's status. I've now rebased with latest successful commit. Can you please trigger the pipeline again?

@Bhargav-InfraCloud Bhargav-InfraCloud requested review from pavannd1 and PrasadG193 and removed request for PrasadG193 and pavannd1 November 23, 2022 09:19
@mergify mergify bot merged commit 964a950 into kanisterio:master Nov 29, 2022
@Bhargav-InfraCloud Bhargav-InfraCloud deleted the gofmt-fixes branch November 29, 2022 07:52
@Bhargav-InfraCloud
Copy link
Contributor Author

Thanks again @PrasadG193 @pavannd1 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix linter issues per Go Report
3 participants