ci: upgrade golangci-lint to v2 #746
Conversation
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
There was a problem hiding this comment.
Summary of Changes
Hello @erka, 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 a significant infrastructure update by upgrading the golangci-lint tool to its second major version. This upgrade ensures the project maintains up-to-date static analysis capabilities and promotes consistency with other related repositories. Beyond the linter upgrade, the changes encompass a series of code quality improvements, refactorings, and enhanced error handling practices across various provider implementations, contributing to a more robust and maintainable codebase.
Highlights
- Linter Upgrade: Upgraded
golangci-lintto version 2, including a complete rewrite of the.golangci.yamlconfiguration to align with the new version's structure and features. This involved enabling a curated set of linters and temporarily disabling others for a smoother transition. - Build Process Updates: Modified the
Makefileto correctly install and execute the newgolangci-lintversion, utilizing new variables for version management and adjusting the execution path fromGOPATHtoGOROOT. - Code Quality & Readability Improvements: Addressed various linter-reported issues across multiple provider packages. This includes refactoring
if-elsechains intoswitchstatements for enhanced readability, adding_ =to ignore return values fromClose()methods to satisfy error-checking linters, and updating comments for improved clarity. - Error Handling Enhancements: Improved error handling in several areas, such as adding
require.NoError(t, err)andrequire.Error(t, err)in test files, and logging errors when closing response bodies in HTTP-related utilities. - Code Cleanup: Removed unused functions (e.g.,
toStrinproviders/prefab) and added//nolint unusedcomments to fields that are intentionally unused but kept for future functionality, reducing code clutter and suppressing linter warnings.
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.
Code Review
This pull request successfully upgrades golangci-lint to v2 and addresses the linter issues found in the codebase. The changes are mostly small fixes, such as handling unchecked errors, removing unused code, and applying stylistic improvements, which enhance code quality. The refactoring of if-else chains to switch statements and the improvements to test files (e.g., using go:embed) are also positive changes.
I've identified one critical issue in the Makefile that will break the lint command, and a medium-severity suggestion to reduce code duplication. After addressing the critical issue, this PR will be in great shape.
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
| logger *slog.Logger | ||
| publishEvents bool | ||
| metadata *of.Metadata | ||
| metadata *of.Metadata //nolint unused |
There was a problem hiding this comment.
if it is unused should we keep the configuration?
There was a problem hiding this comment.
That’s a good point. Since the multi-provider has been re-implemented in the go-sdk, it probably doesn’t matter much now as it will be removed soon.
| // EnvironmentNames []string | ||
| // ProjectEnvID int64 | ||
| // InitializationTimeoutSeconds float64 | ||
| // OnInitializationFailure OnInitializationFailure | ||
| //nolint OnInitializationFailure OnInitializationFailure |
There was a problem hiding this comment.
Should we keep this if it is unused ?
There was a problem hiding this comment.
this is dupword. I don't want to remove it as I have no idea why those comments are there.
| err = of.SetProviderAndWait(provider) | ||
| if err != nil { | ||
| fmt.Printf("Error during provider set: %v\n", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
Do we really want to crash the test with os.Exit ?
It will probably better to assert here rather than crashing the full test suite.
There was a problem hiding this comment.
Ya we are also exiting above already.
| if err != nil { | ||
| fmt.Printf("Error during removing backup directory: %v\n", err) | ||
| } | ||
| os.Exit(exitCode) |
There was a problem hiding this comment.
Same remarks as https://github.com/open-feature/go-sdk-contrib/pull/746/files#r2432750847, should we os.Exit here?
There was a problem hiding this comment.
This is TestMain(m *testing.M).
| //go:embed demo_app_toggles.json | ||
| var appToggles []byte |
There was a problem hiding this comment.
I'm curious if the linter recommended this or if you just found it.
There was a problem hiding this comment.
My crime 😅 I didn't want to call os.Exit.
This PR
Related Issues
Notes
The motivation is to keep the same version and configuration of golangci-lint between the
go-sdkandgo-sdk-contribrepositories.Some linters and rules have been temporarily disabled to simplify the upgrade process and will be re-enabled separately.