Skip to content

.golangci.yml: simplify#2566

Merged
mtrmac merged 1 commit intocontainers:mainfrom
kolyshkin:golang-yml
Apr 10, 2025
Merged

.golangci.yml: simplify#2566
mtrmac merged 1 commit intocontainers:mainfrom
kolyshkin:golang-yml

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin commented Apr 4, 2025

  1. Remove some unused exclusions, document the used ones.
  2. Add errorlint, fix found warnings ("Use %w to format errors"). (dropped)

@kolyshkin kolyshkin changed the title .golangci.yml: simplify .golangci.yml: simplify, add errorlint, fix its warnings Apr 4, 2025
Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

I’m generally very skeptical of detailed linter configuration given our limited capacity (containers/image#2244 ), because that’s committing us to a long-term maintenance effort.

I’d prefer not to deviate from defaults if at all possible.

Comment thread .golangci.yml
checks:
- all
- -ST1000 # Incorrect or missing package comment.
- -ST1005 # Incorrectly formatted error string.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not the same thing as the previous one. I don’t really want to be in the business of micromanaging this.

(Before version: "2", the configuration used to start with inherit, with us adding only the exceptions, but that no longer works for some reason.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is not the same thing as the previous one.

Right, it's not. As per commit message, this patch removes some unused exclusions.

My thinking is, it makes no sense to disable warnings which we never hit. If and when we hit those warnings, we can decide on a case by case basis whether we prefer to exclude or fix those.

As to "let's stick to golangci-lint defaults" mantra, I generally agree, but in this case we clearly already don't follow it. Since we deviate, it's probably best to minimize the linter configuration.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If and when we hit those warnings, we can decide on a case by case basis whether we prefer to exclude or fix those.

= committing to spend more time on this in the future…

… Fine. Would documenting all # Compared to Golangci-lint 2.0.2 defaults, we don’t exclude "-ST1003", "-ST1016", "-ST1020", "-ST1021", "-ST1022" because we don’t currently hit those work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Comment thread .golangci.yml Outdated
Comment thread .golangci.yml
@kolyshkin
Copy link
Copy Markdown
Contributor Author

Second commit removed.

@packit-as-a-service
Copy link
Copy Markdown

Ephemeral COPR build failed. @containers/packit-build please check.

@kolyshkin kolyshkin changed the title .golangci.yml: simplify, add errorlint, fix its warnings .golangci.yml: simplify Apr 8, 2025
Remove some unused exclusions, document the used ones.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@lsm5
Copy link
Copy Markdown
Member

lsm5 commented Apr 10, 2025

The Packit rawhide failures should go away once the copr buildroot gets a new libgpg-error.

Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

(And thanks @lsm5 for keeping an eye on the build failures.)

@mtrmac mtrmac merged commit a6ff545 into containers:main Apr 10, 2025
30 of 32 checks passed
@stale-locking-app stale-locking-app Bot locked as resolved and limited conversation to collaborators Jul 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants