-
Notifications
You must be signed in to change notification settings - Fork 122
gomod: update to Go 1.24.6 and golangci-lint v2
#993
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @hieblmi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on essential maintenance by upgrading the project's Go version to 1.24.6 and updating the golangci-lint tool to its latest v2 release. These updates necessitate a refactoring of the linter configuration file and several minor code adjustments to comply with the new linter rules and improve overall code quality and readability. The changes ensure the project benefits from the latest language features, performance enhancements, and robust static analysis capabilities.
Highlights
- Go Version Upgrade: The Go programming language version has been upgraded from 1.23.12 to 1.24.6, ensuring compatibility with the latest language features and performance improvements.
- Linter Upgrade and Configuration Refactor: The static analysis tool 'golangci-lint' has been updated to version 2.4.0, along with a comprehensive migration of its configuration file (.golangci.yml) to the new v2 format. This includes restructuring settings and exclusions.
- Lint Issue Resolution and Code Style Improvements: Numerous minor code style and error handling adjustments have been made across various Go files to resolve issues flagged by the updated linter. This includes changes to 'if err := ...' patterns, mutex declarations, and adding readability-enhancing blank lines.
- Development Environment Updates: The project's Dockerfile for development tools has been updated to reflect the new Go version and linter changes, streamlining the build environment.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully upgrades the Go version to 1.24.6 and migrates the golangci-lint configuration to the v2 format. The changes are well-executed, including dependency updates and numerous lint fixes that improve code quality and maintainability.
Key improvements include:
- Correcting a bug in
loopd/daemon.gowhere an incorrect error variable was being logged. - Fixing several instances of variable shadowing.
- Improving error handling by using
errors.Isinrouting_plugin.go. - Reordering struct fields for better memory alignment.
Overall, this is a solid maintenance PR that brings the project up to date with its tooling and dependencies. I have one specific comment on the bug fix.
| startErr := d.startWebServers() | ||
| if startErr != nil { | ||
| errorf("Error while starting daemon: %v", err) | ||
| errorf("Error while starting daemon: %v", startErr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change corrects a bug where the wrong error variable was being logged. The original code logged err, which is assigned in a separate goroutine and could be nil or stale here, instead of startErr which holds the actual error from d.startWebServers(). This fix ensures the correct error is logged, which is crucial for debugging.
1252fc8 to
4f877dd
Compare
.golangci.v1.bck.yml
Outdated
| @@ -0,0 +1,149 @@ | |||
| run: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep the v1 file around as a backup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need them, we can always just restore right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would be able to recover it from Git if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted the file
4ba877d to
7d9c650
Compare
847f2ef to
17e1945
Compare
starius
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 📈
Thanks for updating it! I left several comments.
loopd/daemon.go
Outdated
| swapClientServer | ||
|
|
||
| // To be used atomically. | ||
| started int32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use atomic.Bool? (In a separate commit.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added commit f97c18d
| # If you change this value, please change it in the following files as well: | ||
| # /Dockerfile | ||
| GO_VERSION: 1.24.0 | ||
| GO_VERSION: 1.24.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo in the commit message: "upgarde"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tools/Dockerfile
Outdated
| && chmod -R 777 /tmp/build/ | ||
| RUN mkdir -p /tmp/build/.cache /tmp/build/.modcache \ | ||
| && cd /tmp/tools \ | ||
| && go install -trimpath github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.4.0 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@v2.4.0 is not needed, because the version is fixed in go.mod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
executor.go
Outdated
|
|
||
| // executor is responsible for executing swaps. | ||
| // | ||
| // TODO(roasbeef): rename to SubSwapper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we this TODO is outdated and can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
liquidity/liquidity.go
Outdated
| // Run periodically checks whether we should automatically dispatch a loop out. | ||
| // We run this loop even if automated swaps are not currently enabled rather | ||
| // than managing starting and stopping the ticker as our parameters are updated. | ||
| // than managing to start and stop the ticker as our parameters are updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We run this loop even if automated swaps are not currently enabled, instead of starting and stopping the ticker whenever our parameters are updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better, updated
staticaddr/deposit/deposit.go
Outdated
| // Outpoint of the deposit. | ||
| wire.OutPoint | ||
|
|
||
| sync.Mutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move the mutex to the beginning. Maybe it also makes sense to make wire.OutPoint non-anonymous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
bhandras
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉 Thank you!
.golangci.v1.bck.yml
Outdated
| @@ -0,0 +1,149 @@ | |||
| run: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need them, we can always just restore right?
f97c18d to
35be4c0
Compare
go is updated to v1.24.6 and golangci from v1.64.6 to v2.4.0.
Migrated with `golangci-lint migrate --skip-validation`
fixes tests TestLoopOutFailOffchain, TestLoopOutPaymentParameters
This PR upgrades loop's go version to
1.24.6, upgrades the linter tov2.4.0and migrates the.golangci.ymlfile and fixes lint issues.