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 revive linter #3241

Merged
merged 7 commits into from
May 21, 2024
Merged

Enable revive linter #3241

merged 7 commits into from
May 21, 2024

Conversation

EduardGomezEscandell
Copy link
Collaborator

What this PR does / why we need it:
Enables the revive linter:

Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint.

Golint itself has been deprecated for 3 years.

Special notes for your reviewer:
PR #3193 removes golint from the make lint-metricstarget.

Release note:

NONE

@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

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. 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 May 3, 2024
@EduardGomezEscandell EduardGomezEscandell changed the title Linter revive Enable revive linter May 3, 2024
@EduardGomezEscandell
Copy link
Collaborator Author

/test all

@kubevirt-bot kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels May 6, 2024
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels May 6, 2024
@EduardGomezEscandell EduardGomezEscandell force-pushed the linter-revive branch 2 times, most recently from d7fc229 to 5f6fdd2 Compare May 15, 2024 07:54
@EduardGomezEscandell
Copy link
Collaborator Author

/test all

@EduardGomezEscandell EduardGomezEscandell force-pushed the linter-revive branch 2 times, most recently from fb8d127 to 89eb44f Compare May 16, 2024 08:16
@EduardGomezEscandell
Copy link
Collaborator Author

/test all

@coveralls
Copy link

coveralls commented May 16, 2024 •
edited
Loading

Coverage Status

coverage: 58.5% (+41.3%) from 17.245%
when pulling 6fd824d on EduardGomezEscandell:linter-revive
into 7a49da9 on kubevirt:main.

@EduardGomezEscandell
Enable revive linter
1c5163e 
Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell
Simplify cdi-func-test-proxy
d7f7cbe 
This function had quite a bit of redundant code (caught by the linter).
The workgroup was never Done because all exit paths went through a
log.Fatal.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell
Fix 'revive' linter warnings
97273f8 
Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell
Fix tests that asserted on modified error messages
61464e5 
Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell
Run make format
ed4937b 
The formatted code has nothing to do with this PR but we may as well
include it.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell EduardGomezEscandell marked this pull request as ready for review May 17, 2024 09:31
@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 May 17, 2024
awels
awels reviewed May 20, 2024
View reviewed changes
pkg/importer/vddk-datasource_test.go Outdated
@@ -666,14 +666,14 @@ var _ = Describe("VDDK get block status", func() {
flagSetting := true
currentMockNbdFunctions.BlockStatus = func(length uint64, offset uint64, callback libnbd.ExtentCallback, optargs *libnbd.BlockStatusOptargs) error {
err := 0
len := uint32(MaxBlockStatusLength)
L := uint32(MaxBlockStatusLength)
Copy link
Member

Choose a reason for hiding this comment

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

why use an upper case L instead of a lower case one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The linter dislikes shadowing built-in variables and functions such as len, max, etc.

The uppercase L is just my lack of originality

Copy link
Member

Choose a reason for hiding this comment

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

Right it is just that normally upper cass variables indicate 'public' type variables. Which of course is not a thing inside a function, it is just confusing. I would just use lower case l and we can call it a day.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good.

tools/cdi-func-test-proxy/main.go Outdated
startServer(httpsPortWithAuth, withBasicAuth, true, wg)

wg.Wait()
select {}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel semantically the same as the wait groups. Am I missing something? Also I think the log.Fatal in startServer is probably a mistake on my part.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both things block forever until one of the startServer exits the process with log.Fatal. The calls to wg.Done() are unreachable, so this wg was overkill.

I can rework this to not use log.Fatal and go back to workgroups if you prefer. In that case they wouldn't be overkill.

Copy link
Member

Choose a reason for hiding this comment

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

IMO it should not exit until all the threads have finished and released their resources. So the original code is faulty as well. I would remove the log.Fatal. Not sure why I did that in the first place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense, yeah. Using some context cancellation that is easy to achieve.

@EduardGomezEscandell
Use lower-case variables and use built-in min function in vddk-dataso…
50915a2 
…urce

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
@EduardGomezEscandell
Use contexts in cdi-func-test-proxy
6fd824d 
This added quite a bit of boilerplate per call, so I put everything in
a loop.

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

awels commented May 21, 2024

/test pull-containerized-data-importer-e2e-istio

@awels
Copy link
Member

awels commented May 21, 2024

/test pull-containerized-data-importer-fossa

awels
awels approved these changes May 21, 2024
View reviewed changes
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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 May 21, 2024
@kubevirt-bot kubevirt-bot merged commit cd7c8b1 into kubevirt:main May 21, 2024
19 checks passed
@EduardGomezEscandell EduardGomezEscandell deleted the linter-revive branch May 21, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awels awels awels approved these changes

@aglitke aglitke Awaiting requested review from aglitke

@arnongilboa arnongilboa Awaiting requested review from arnongilboa

@akalenyu akalenyu Awaiting requested review from akalenyu

Assignees

@awels awels

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-none Denotes a PR that doesn't merit a release note. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@EduardGomezEscandell @kubevirt-bot @coveralls @awels