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.

Sign up for GitHub

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

Jump to bottom

Enable autoformatting linters #3179

Merged
merged 6 commits into from
Apr 24, 2024
Merged

Enable autoformatting linters #3179

merged 6 commits into from
Apr 24, 2024

Conversation

EduardGomezEscandell
Copy link
Collaborator

What this PR does / why we need it:
This PR adds a few linters that autoformat the code. This should help make the codebase more consistent in style, as is the Go way.

  • Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification.
  • Whitespace is a linter that checks for unnecessary newlines at the start and end of functions, if, for, etc.
  • Gci controls Go package import order and makes it always deterministic. This one produces a big diff though.

How to run them
In CI runs, these will not autoformat and instead will just complain. Locally, you can run them with:

golangci-lint run ./... --fix

If you're using VSCode, you can add these options so the it runs when you save. I'm sure other editors have similar options.

"go.lintOnSave": "file", // Optional, makes it faster
"go.lintTool": "golangci-lint",
"go.lintFlags": [ "--fix" ],

Special notes for your reviewer:
The only manual changes are ones made to .golangci.yaml. Everythin else is auto-formatted.
I recommend reviewing this PR commit by commit as the result is very large.

Release note:

Enable autoformatting linters and format the project

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Apr 10, 2024
@kubevirt-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@EduardGomezEscandell
Copy link
Collaborator Author

If the amount of changes is too scary, I can get rid of the GCI linter as that's where the majority of them come from.

@alromeros
Copy link
Collaborator

Thanks for the PR! This improves consistency and looks good to me, let's see what others think.

@EduardGomezEscandell EduardGomezEscandell marked this pull request as ready for review April 10, 2024 13:09
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 10, 2024
@EduardGomezEscandell
Copy link
Collaborator Author

/retest

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2024
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2024
@EduardGomezEscandell
Copy link
Collaborator Author

/retest

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024
@mhenriks
Copy link
Member

Locally, you can run them with:
golangci-lint run ./... --fix

Can we add a make target that runs this in the builder container?

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024
@EduardGomezEscandell
Copy link
Collaborator Author

Can we add a make target that runs this in the builder container?

Already on it #3193 😃

The idea is to add this to make format.

@kubevirt-bot kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 19, 2024
@EduardGomezEscandell
Enable gofmt linter
538af56 
From the docs:

> Gofmt checks whether code was gofmt-ed. By default this tool runs with
> -s option to check for code simplification.

https://golangci-lint.run/usage/linters/#gofmt

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell
Run gomft on the project
2edfce0 
Ran this command after adding the gofmt linter:

	golangci-lint run ./... --fix

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell
Enable whitespace linter
73f3b13 
From the docs:
> Whitespace is a linter that checks for unnecessary newlines at the
> start and end of functions, if, for, etc.

https://golangci-lint.run/usage/linters/#whitespace

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell
Run whitespace on the project
07a2db7 
Ran this command after adding the whitespace linter:

	golangci-lint run ./... --fix

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell
Enable GCI linter
73d8006 
Per the docs:

> Gci controls Go package import order and makes it always deterministic.

https://golangci-lint.run/usage/linters/#gci

NOTE: I noticed that many files separate their imports in a particular
way, so I set the linter to enforce this standard.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell
Run GCI on the project
772791a 
Ran this command after adding the GCI linter:

	golangci-lint run ./... --fix

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2024
@mhenriks
Copy link
Member

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2024
@alromeros
Copy link
Collaborator

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2024
@kubevirt-bot kubevirt-bot merged commit 0e75026 into kubevirt:main Apr 24, 2024
17 of 18 checks passed
@EduardGomezEscandell EduardGomezEscandell deleted the linters-autoformat branch April 24, 2024 11:52
@EduardGomezEscandell EduardGomezEscandell mentioned this pull request Apr 24, 2024
Merged
7 tasks
@akalenyu akalenyu mentioned this pull request Apr 26, 2024
Merged
3 tasks
akalenyu pushed a commit to akalenyu/containerized-data-importer that referenced this pull request May 1, 2024
@EduardGomezEscandell @akalenyu
Enable autoformatting linters (kubevirt#3179)
e1ac49d 
* Enable gofmt linter

From the docs:

> Gofmt checks whether code was gofmt-ed. By default this tool runs with
> -s option to check for code simplification.

https://golangci-lint.run/usage/linters/#gofmt

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Run gomft on the project

Ran this command after adding the gofmt linter:

	golangci-lint run ./... --fix

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Enable whitespace linter

From the docs:
> Whitespace is a linter that checks for unnecessary newlines at the
> start and end of functions, if, for, etc.

https://golangci-lint.run/usage/linters/#whitespace

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Run whitespace on the project

Ran this command after adding the whitespace linter:

	golangci-lint run ./... --fix

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Enable GCI linter

Per the docs:

> Gci controls Go package import order and makes it always deterministic.

https://golangci-lint.run/usage/linters/#gci

NOTE: I noticed that many files separate their imports in a particular
way, so I set the linter to enforce this standard.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Run GCI on the project

Ran this command after adding the GCI linter:

	golangci-lint run ./... --fix

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

---------

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
kubevirt-bot added a commit that referenced this pull request May 3, 2024
@akalenyu @EduardGomezEscandell
@mhenriks @kubevirt-bot
[release-v1.59] Catch up with non-logical main changes for ease of ma…
8b25da0 
…intenance (#3227)

* Double number of e2e parallel nodes from 3 to 6 (#3156)

We have some feedback from different platform testing that this
can work tests (not resource) wise. Let's give it a try here as well

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Support IPv6 in controller GetMetricsURL (#3165)

Joining hostname+port with string concatenation leads to wrong URLs
when the hostame is an IPv6:

HOST    PORT   Sprintf    CORRECT
::1     8000   ::1:8000   [::1]:8000

To avoid this issue, it's best to use net.JoinHostPort. I added a test
that verifies this fix.

This type of issue can be discovered running the following command:

    golangci-lint run ./... --enable nosprintfhostport

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Enable misspell linter and fix spelling errors (#3164)

* Add misspell to list of linters

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Fix spelling errors

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

---------

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Remove openshift dep on api (#3172)

Less deps the better!  Could be problematic for projects importing CDI and openshift.

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

* Enable unconvert linter (#3171)

* Enable unconvert linter

This linter's doc describes it as:

   The unconvert program analyzes Go packages to identify unnecessary
   type conversions; i.e., expressions T(x) where x already has type T.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Unrestrict the number of linter warnings

It is best to show all warnings at once than to reveal them piece-meal,
particularly in CI where the feedback loop can be a bit slow.

By default, linters may only print the same message three times
(https://golangci-lint.run/usage/configuration/#issues-configuration)
The unconvert linter always prints the same message, so it specially
affected by this setting.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Remove redundant type conversions

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

---------

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Enable linters checking error handling (#3177)

* Enable errorlint linter

Per the readme:

> go-errorlint is a source code linter for Go software that can be used
to find code that will cause problems with the error wrapping scheme
introduced in Go 1.13.

https://github.com/polyfloyd/go-errorlint

Essentially, it checks that you properly use errors.Is and errors.As

It also forces you to use %w, but that is a bit too much. Using %v is
fine when you don't want to leak the internal error types of the
package. This is why I disabled this setting.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Replace error comparisons with errors.Is

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Replace error type assertions with errors.As

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Enable errname linter

Per the readme:
> Checks that sentinel errors are prefixed with the Err and error
> types are suffixed with the Error.

https://github.com/Antonboom/errname

Not a big deal, but helps keep the naming consistent.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Fix errname linter warnings

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

---------

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Run bazelisk run //robots/cmd/uploader:uploader -- -workspace /home/prow/go/src/github.com/kubevirt/project-infra/../containerized-data-importer/WORKSPACE -dry-run=false (#3199)

Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com>

* CVE 2024-24786 fix: Bump google.golang.org/protobuf from 1.31.0 to 1.33.0 (#3195)

* Upgrade google.golang.org/protobuf

This solves CVE-2024-24786

https://www.cve.org/CVERecord?id=CVE-2024-24786

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Update checksum and vendoring

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

---------

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Enable durationcheck linter (#3176)

* Enable durationcheck linter

> Check for two durations multiplied together.

https://github.com/charithe/durationcheck

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Fix misuse of time.Duration

I also had to rename the variable because go-statickcheck complains
about a time.Duration having the suffix 'Sec'.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

---------

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Enable nakedret linter (#3178)

* Enable nakedret linter

> Checks that functions with naked returns are not longer than a maximum
> size (can be zero).

https://github.com/alexkohler/nakedret

Relevant from the GO style guide:
> Naked returns are okay if the function is a handful of lines. Once
> it’s a medium sized function, be explicit with your return values.

https://go.dev/wiki/CodeReviewComments#named-result-parameters

I think a naked return is never easier to read than a regular return,
so I set the linter to always complain about it. Disagreements are
welcome.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Refactor functions with naked returns

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

---------

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Clean up leftover snapshot sources from DataImportCron tests (#3123)

* Clean up leftover snapshot sources from DataImportCron tests

The dataimportcron tests may affect existing crons in the environment (in addition to the ones deployed by testing)
so we are better off cleaning up after ourselves.

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Add watch for volumesnapshot delete

Although we don't support seamless transition from volumesnapshot->pvc sources
(we hope you stay on snapshots if they scale better for your storage)
this will still be needed in most cases when someone switches and manually deletes snapshots.

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

---------

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Enable autoformatting linters (#3179)

* Enable gofmt linter

From the docs:

> Gofmt checks whether code was gofmt-ed. By default this tool runs with
> -s option to check for code simplification.

https://golangci-lint.run/usage/linters/#gofmt

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Run gomft on the project

Ran this command after adding the gofmt linter:

	golangci-lint run ./... --fix

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Enable whitespace linter

From the docs:
> Whitespace is a linter that checks for unnecessary newlines at the
> start and end of functions, if, for, etc.

https://golangci-lint.run/usage/linters/#whitespace

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Run whitespace on the project

Ran this command after adding the whitespace linter:

	golangci-lint run ./... --fix

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Enable GCI linter

Per the docs:

> Gci controls Go package import order and makes it always deterministic.

https://golangci-lint.run/usage/linters/#gci

NOTE: I noticed that many files separate their imports in a particular
way, so I set the linter to enforce this standard.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Run GCI on the project

Ran this command after adding the GCI linter:

	golangci-lint run ./... --fix

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

---------

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Enable dupword linter (#3175)

* Enable dupword linter

Per the readme:
> A linter that checks for duplicate words in the source code (usually miswritten)

https://github.com/Abirdcfly/dupword
https://golangci-lint.run/usage/linters/#dupword

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Remove duplicate words in comments and strings

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

---------

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Change clone strategy and cron sources format for LVMS (#3196)

Following some investigation we are now confident this tuning is benefical for LVMS:
- CSI clone is effectively implemented the same as snapshotting because both already use lvm2 thinly provisioned snapshots under the hood
- Snapshots being COW, it makes total sense to store cron sources in the snapshot format instead of PVC:
```
A snapshot of a thin logical volume also creates a thin logical volume.
This consumes no data space until a COW operation is required, or until the snapshot itself is written.
```

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Set default clone strategy & cron format for trident to snapshot (#3209)

There's a bug in the trident CSI driver that causes a snapshot
to be left over following each CSI clone:
NetApp/trident#901
This is becoming a problem rapidly once one hits the snapshot limit per volume (golden image):
https://kb.netapp.com/onprem/ontap/os/Maximum_number_of_snapshots_supported_by_ONTAP

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

---------

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com>
Co-authored-by: Edu Gómez Escandell <edu1997xyz@gmail.com>
Co-authored-by: Edu Gómez Escandell <egomez@redhat.com>
Co-authored-by: Michael Henriksen <mhenriks@redhat.com>
Co-authored-by: kubevirt-bot <kubevirtbot@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aglitke aglitke Awaiting requested review from aglitke

@awels awels Awaiting requested review from awels

@akalenyu akalenyu Awaiting requested review from akalenyu

@alromeros alromeros Awaiting requested review from alromeros

Assignees

@alromeros alromeros

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@EduardGomezEscandell @kubevirt-bot @alromeros @mhenriks