Bump linter for go1.25 compatibility. apply linting fixes #1849
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dricross
commented
Sep 5, 2025
Comment on lines
-61
to
-65
| # Package name based on field in JSON | ||
| - path: 'translator/translate/logs/logs_collected/windows_events' | ||
| text: "var-naming: don't use an underscore in package name" | ||
| linters: | ||
| - revive |
Contributor
Author
There was a problem hiding this comment.
Didn't realize this exception was in the original config and wasn't moved in the migration until after I fixed it.
| #Install from source for golangci-lint is not recommended based on https://golangci-lint.run/usage/install/#install-from-source so using binary | ||
| #installation | ||
| curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(TOOLS_BIN_DIR) v1.64.2 | ||
| curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(TOOLS_BIN_DIR) v2.4.0 |
Contributor
Author
There was a problem hiding this comment.
master -> HEAD more closely aligns with recommendation: https://golangci-lint.run/docs/welcome/install/#binaries
| "github.com/prometheus/common/promslog" | ||
| "github.com/prometheus/common/version" | ||
| "github.com/prometheus/prometheus/config" | ||
| promconfig "github.com/prometheus/prometheus/config" |
Contributor
Author
There was a problem hiding this comment.
already imported in the preceding line
Comment on lines
-387
to
+396
| //When the specified key is "*", there is no way for us to check if all | ||
| //tags are fetched. So there is no need to do refresh in this case. | ||
| needRefresh = !(len(t.EC2InstanceTagKeys) == 1 && t.EC2InstanceTagKeys[0] == "*") | ||
| needRefresh = len(t.EC2InstanceTagKeys) != 1 || t.EC2InstanceTagKeys[0] != "*" |
Contributor
Author
There was a problem hiding this comment.
Please check my De Morgan's application. Feels like something that's easy to mess up.
| @@ -35,20 +35,13 @@ type CollectList struct { | |||
| } | |||
|
|
|||
| var customizedJSONConfigKeys = []string{"event_name", EventLevelsKey} | |||
| var eventLevelMapping = map[string]string{ | |||
dricross
force-pushed
the
dricross/lintbump
branch
from
7981779 to
140f26d
Compare
Contributor
Author
|
Ran into too many issues with various linting issues and flaky unit tests to solve reasonably quickly. Ended up reverting the Go bump: #1851. When we re-do the Go bump we'll look into these. |
Add Linux build constraint to constants.go to match the build constraints of the files that use these constants. The constants are only used in Linux-specific code (metrics_unix.go and util_unix.go), so they should only be compiled on Linux systems. This resolves the 'unused' linter warnings that occurred when running golangci-lint on non-Linux systems.
Replace the Linux build constraint with specific nolint:unused comments for each constant. This approach: - Keeps constants available across all platforms - Suppresses false positive unused warnings from golangci-lint - Is more explicit about why the linter warnings are being ignored - Maintains better code clarity with explanatory comments The constants are used in Linux-specific files (metrics_unix.go and util_unix.go) but appear unused when linting on other platforms.
Revert the test changes that replaced assert.InDelta with assert.Equal. The original code correctly used InDelta with epsilon tolerance for floating-point comparisons, which is necessary when dealing with mathematical calculations that may have small precision differences across Go versions or platforms. The assert.Equal change was causing test failures due to tiny precision differences in floating-point calculations (e.g., 13.656854249492378 vs 13.65685424949238). Using InDelta with appropriate epsilon tolerance is the correct approach for floating-point test assertions.
- Reset go.mod to main branch baseline - Update Go version from 1.24.9 to 1.25.5 - Run go mod tidy to update dependencies - Maintains all previous linting fixes for nvme constants
- Fix package naming issues with nolint comments for existing packages - Apply De Morgan's law optimizations in kafkabroker extract - Fix error string capitalization and punctuation issues - Remove duplicate import in downloader - Replace strings.Replace with strings.ReplaceAll for efficiency All 13 linting issues from golangci-lint v2.4.0 are now resolved.
the-mann
force-pushed
the
dricross/lintbump
branch
from
d7e5ab7 to
665589f
Compare
Add nolint:revive comments to all files in internal/detector/util package: - process.go - util.go - scan.go - process_test.go - scan_test.go This addresses the CI/CD linting issue that wasn't caught locally due to golangci-lint file discovery inconsistencies.
- Add exclusions for meaningless package names and naming conventions - Remove nolint comments in favor of proper exclusions - Fix TlsKey -> TLSKey naming convention - Remove duplicate TLSKey constant definition - Apply gofmt formatting All linting issues are now resolved with proper exclusions rather than suppressions.
- Restored nolint:revive comments for package naming issues - Restored nolint:revive comments for constant naming issues - Removed golangci-lint exclusions added in this branch - Applied gofmt formatting - Keep TLSKey naming fix and duplicate constant removal All linting issues suppressed with inline nolint comments.
the-mann
added
the
ready for testing
sky333999
reviewed
Dec 15, 2025
tool/cmdwrapper/cmdwrapper.go
Outdated
| var exitErr *exec.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| return fmt.Errorf("E! %s process exited with non-zero status: %d", command, exitErr.ExitCode()) | ||
| return fmt.Errorf("e! %s process exited with non-zero status: %d", command, exitErr.ExitCode()) |
Contributor
There was a problem hiding this comment.
I think we use the E! in a lot more places in the code - so Id keep it as is for now for consistency.
Contributor
There was a problem hiding this comment.
agreed, not sure why the linter changed this one. i'll find which rule changed this and disable it
- Restore uppercase E! in cmdwrapper.go for consistency with codebase - Add global exclusion for ST1005 staticcheck rule to preserve established error logging convention - Remove need for individual nolint comments throughout codebase
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the issue
Check lint is failing on PRs:
We recently bumped to go1.25 but apparently that PR skipped the lint check so it didn't catch this incompatibility 🫠 We have to upgrade from v1.64.2 all the way to v2.4.0 since that's when go1.25 support was added, which incurred some new linting rules.
The upgrade from v1.x to v2.x comes with a new format for the
.golangci-lint.ymlfile. Migrated to v2 format using the following command:Output of
make lintafter upgrading and migrating:Another lint error showed up after the PR check ran:
More lint issues showed up after adjusting the
windows_eventspackage name:Description of changes
License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
make lintRequirements
Before commiting your code, please do the following steps.
make fmtandmake fmt-shmake lintIntegration Tests
To run integration tests against this PR, add the
ready for testinglabel.