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

Update golangci, trying to fix lint #1509

Merged
merged 3 commits into from
Oct 22, 2021
Merged

Update golangci, trying to fix lint #1509

merged 3 commits into from
Oct 22, 2021

Conversation

Kay-Zee
Copy link
Member

@Kay-Zee Kay-Zee commented Oct 21, 2021

This updates the golangci action to just use the latest v2 action, which is recommended for the action.

Also, skipping the PKG cache because of all the File Exists error. Will have to follow: golangci/golangci-lint-action#244 to see when this is resolved. (though lint is one of the faster jobs, so it's fine even if we skip the cache)

Lastly, I'm suggesting we skip the unused lint for TEST FILES ONLY, as there are many cases where we want to use t.Skip() to quarantine, but then it causes "unused" errors. This will resolve those cases and better allow us to use t.Skip()

@Kay-Zee Kay-Zee force-pushed the kan/fix-lint branch 3 times, most recently from 4a70353 to b5bad68 Compare October 21, 2021 06:00
Copy link
Member

@zhangchiqing zhangchiqing left a comment

Choose a reason for hiding this comment

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

Nice. Thanks for the fix!

@tarakby
Copy link
Contributor

tarakby commented Oct 21, 2021

I noticed the linter in makefile is not used by the linter job in CI. This means developers might get different results locally using make and on CI. Isn't better to make CI use make lint instead of the action?

Copy link
Contributor

@gomisha gomisha left a comment

Choose a reason for hiding this comment

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

Good idea to disable unused linter for (skipped) tests

@Kay-Zee
Copy link
Member Author

Kay-Zee commented Oct 22, 2021

I noticed the linter in makefile is not used by the linter job in CI. This means developers might get different results locally using make and on CI. Isn't better to make CI use make lint instead of the action?

The CI action is nice to have, as it gives more features, and allows slightly more control (also, the golangci team obviously suggests using golangci action instead of your own)

They do use the same config file, so should return the same results

@Kay-Zee
Copy link
Member Author

Kay-Zee commented Oct 22, 2021

bors merge

bors bot added a commit that referenced this pull request Oct 22, 2021
1509: Update golangci, trying to fix lint r=Kay-Zee a=Kay-Zee

This updates the golangci action to just use the latest v2 action, which is recommended for the action.

Also, skipping the PKG cache because of all the File Exists error. Will have to follow: golangci/golangci-lint-action#244 to see when this is resolved. (though lint is one of the faster jobs, so it's fine even if we skip the cache)

Lastly, I'm suggesting we skip the `unused` lint for TEST FILES ONLY, as there are many cases where we want to use `t.Skip()` to quarantine, but then it causes "unused" errors. This will resolve those cases and better allow us to use `t.Skip()`

Co-authored-by: Kay-Zee <kan@axiomzen.co>
@bors
Copy link
Contributor

bors bot commented Oct 22, 2021

Build failed:

@Kay-Zee
Copy link
Member Author

Kay-Zee commented Oct 22, 2021

bors merge

bors bot added a commit that referenced this pull request Oct 22, 2021
1509: Update golangci, trying to fix lint r=Kay-Zee a=Kay-Zee

This updates the golangci action to just use the latest v2 action, which is recommended for the action.

Also, skipping the PKG cache because of all the File Exists error. Will have to follow: golangci/golangci-lint-action#244 to see when this is resolved. (though lint is one of the faster jobs, so it's fine even if we skip the cache)

Lastly, I'm suggesting we skip the `unused` lint for TEST FILES ONLY, as there are many cases where we want to use `t.Skip()` to quarantine, but then it causes "unused" errors. This will resolve those cases and better allow us to use `t.Skip()`

Co-authored-by: Kay-Zee <kan@axiomzen.co>
@bors
Copy link
Contributor

bors bot commented Oct 22, 2021

Build failed:

gomisha added a commit that referenced this pull request Oct 22, 2021
@Kay-Zee
Copy link
Member Author

Kay-Zee commented Oct 22, 2021

bors merge

@bors
Copy link
Contributor

bors bot commented Oct 22, 2021

@bors bors bot merged commit 5413790 into master Oct 22, 2021
@bors bors bot deleted the kan/fix-lint branch October 22, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants