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

golangci: lint with Go 1.21, 1.22, 1.23 and macOS, Linux, Windows #429

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

orangecms
Copy link
Collaborator

No description provided.

@orangecms
Copy link
Collaborator Author

that looks helpful - it tells exactly where the issue is in which file, WDYT @rminnich ?

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.63%. Comparing base (98cc60b) to head (0ad463f).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #429   +/-   ##
=======================================
  Coverage   41.63%   41.63%           
=======================================
  Files         142      142           
  Lines       10729    10729           
=======================================
  Hits         4467     4467           
  Misses       5394     5394           
  Partials      868      868           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@orangecms orangecms mentioned this pull request Sep 16, 2024
@rminnich
Copy link
Contributor

as long as it does not block pushing stuff in, this is ok.

@rminnich
Copy link
Contributor

But this advice is not really that useful:

Check failure on line 19 in pkg/intel/metadata/fit/ent_startup_ac_module_entry_test.go


GitHub Actions
/ lint (1.22, macos-latest)
SA1019: rand.Read has been deprecated since Go 1.20 because it shouldn't be used: For almost all use cases, [crypto/rand.Read] is more appropriate. (staticcheck)

it's a test. I doubt using rand.Read in a test is a problem.

So I'm still not quite convinced.

@orangecms
Copy link
Collaborator Author

It's a linter after all, not magical.

Telling about deprecation is beneficial imho.

It's not blocking anyway... better to have it and be aware if issues such as unhandled errors etc..

@binjip978
Copy link
Collaborator

But this advice is not really that useful:

Check failure on line 19 in pkg/intel/metadata/fit/ent_startup_ac_module_entry_test.go


GitHub Actions
/ lint (1.22, macos-latest)
SA1019: rand.Read has been deprecated since Go 1.20 because it shouldn't be used: For almost all use cases, [crypto/rand.Read] is more appropriate. (staticcheck)

it's a test. I doubt using rand.Read in a test is a problem.

So I'm still not quite convinced.

It possible to use u-root linter configuration https://github.com/u-root/u-root/blob/main/.golangci.yml where we don't block on deprecation issues. Also compared to u-root there are no so much warnings :)

Copy link
Contributor

@rminnich rminnich left a comment

Choose a reason for hiding this comment

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

The consensus being that this is a good idea, let's go!

@orangecms
Copy link
Collaborator Author

Then let me grab the config from u-root to make it non-blocking.

@rminnich rminnich merged commit 58d8581 into linuxboot:main Sep 17, 2024
6 of 15 checks passed
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.

4 participants