Skip to content

Conversation

mmorel-35
Copy link
Contributor

Description

This fixes staticcheck issues in exporter discovered after golangci-lint@v2 upgrade

@douglascamata
Copy link
Member

@mmorel-35 why are we fixing this? Is there some "paper trail" we can follow to be sure that it's something we want to do?

@atoulme atoulme merged commit dd441d1 into open-telemetry:main Apr 7, 2025
172 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 7, 2025
@mmorel-35 mmorel-35 deleted the staticcheck-exporter branch April 7, 2025 15:36
@atoulme
Copy link
Contributor

atoulme commented Apr 7, 2025

@mmorel-35 please make sure to answer this comment: #39192 (comment)

@mmorel-35
Copy link
Contributor Author

Hi @douglascamata,

Fixing these staticcheck issues is part of ongoing efforts to maintain code quality and adhere to best practices. The recent upgrade to golangci-lint@v2 highlighted several issues that need to be resolved.

This change ensures that the codebase remains clean and maintainable, which is crucial for the long-term health of the project.

There is no specific discussion or paper trail available that supports this change. The fixes are based on the options provided by golangci-lint and specially staticcheck for this PR.

@douglascamata
Copy link
Member

douglascamata commented Apr 8, 2025

The recent upgrade to golangci-lint@v2 highlighted several issues that need to be resolved.

Huuuummm, I strongly disagree. Most of the automatic fixes applied here are removal of embed structs when referencing their methods or fields (c.Metrics.TCPAddrConfig.Endpoint = server.URL vs c.Metrics.Endpoint = server.URL) and some occurrences of boolean logic simplification (!(partialSuccess.ErrorMessage() == "" && partialSuccess.RejectedLogRecords() == 0) vs partialSuccess.ErrorMessage() != "" || partialSuccess.RejectedLogRecords() != 0).

These are not "issues that need to be resolved".

This change ensures that the codebase remains clean and maintainable, which is crucial for the long-term health of the project.

All these changes do not necessarily make the codebase "clean and maintainable". For instance, we just lost some clarity on all embed method/field access across the whole codebase. A huge impact of this that wasn't taken into consideration: refactors on the methods/fields of those embed structs were easier when they were explicit (easy to spot them with simple editor tools and change them at scale without mistakes), now this will be a bit more difficult.

There is no specific discussion or paper trail available that supports this change. The fixes are based on the options provided by golangci-lint and specially staticcheck for this PR.

To me this is not the kind of change that should not happen without any previous discussion. We literally flipped something that seems to have been a standard throughout the whole codebase without talking and understanding why or the consequences. "just because the linter said so" might not be a good driver of change.

@douglascamata
Copy link
Member

Plus, we are not enforcing the linter check to pass with golangci-lint@v2 on every PR, no? This means that today people can already start merging code that will trigger more linter warnings just like these that were fixed here. Are there plans to enforce a pass of golangci-lint@v2 at least of the modified files?

dmathieu pushed a commit to dmathieu/opentelemetry-collector-contrib that referenced this pull request Apr 8, 2025
#### Description

This fixes staticcheck issues in exporter discovered after
golangci-lint@v2 upgrade

Signed-off-by: Matthieu MOREL <[email protected]>
@dominikh
Copy link

dominikh commented Apr 9, 2025

golangci-lint@v2 seems to be running Staticcheck's quickfix checks, which are completely optional automatic code changes intended for use in gopls. They let you remove embedded fields from selectors, or apply De Morgan's law to boolean expressions, for example. By no means are these issues that need to be fixed, or changes that should be applied unconditionally.

I'd argue that the majority of changes in this PR shouldn't have been made.

Fiery-Fenix pushed a commit to Fiery-Fenix/opentelemetry-collector-contrib that referenced this pull request Apr 24, 2025
#### Description

This fixes staticcheck issues in exporter discovered after
golangci-lint@v2 upgrade

Signed-off-by: Matthieu MOREL <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants