Upgrade golangci-lint to v2 #8986

nopcoder · 2025-04-27T06:20:18Z

Fixed some of the lint issues - plan to enable lint on tests and more linters after this PR.
The github-actions output format was removed - will consider to use their action to run the lint

handle some of the lint errors next PR will enable more on test code

github-actions · 2025-04-27T06:27:58Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2025-04-27T06:28:52Z

E2E Test Results - Quickstart

no longer supported

arielshaqed

This is really neat! But some of the new(?) linter features are not so good. Please see comments inlines. I did not go over the second half, if it's OK with you I would rather finish handling these comments and then have another round.

arielshaqed · 2025-04-30T07:09:24Z

cmd/lakectl/cmd/import.go

@@ -97,15 +97,17 @@ var importCmd = &cobra.Command{
 			case <-ticker.C:
 				statusResp, err = client.ImportStatusWithResponse(ctx, toURI.Repository, toURI.Ref, &apigen.ImportStatusParams{Id: importID})
 				DieOnErrorOrUnexpectedStatusCode(statusResp, err, http.StatusOK)
-				status := statusResp.JSON200
-				if status == nil {
+				if statusResp.JSON200 == nil {


I don't understand this "fix". It seems longer, more reasonable, and easier to get wrong when code is notified. (TBF, if this flags a "can be nil" warning, any future breakage will flag it again...)

arielshaqed · 2025-04-30T07:11:01Z

cmd/lakectl/cmd/local_init.go

@@ -80,7 +80,7 @@ func localInit(ctx context.Context, dir string, remote *uri.URI, force, updateIg
 		ignoreFile, err := git.Ignore(dir, []string{dir}, []string{filepath.Join(dir, local.IndexFileName)}, local.IgnoreMarker)
 		if err == nil {
 			fmt.Println("Location added to", ignoreFile)
-		} else if !(errors.Is(err, giterror.ErrNotARepository) || errors.Is(err, giterror.ErrNoGit)) {
+		} else if !errors.Is(err, giterror.ErrNotARepository) && !errors.Is(err, giterror.ErrNoGit) {


Why is this better? Can we turn this longer off?
I definitely prefer the former (but not enough to require it). For instance, it is easier to refactor into a function isErrNotVersiomed. In general, negatives are confusing, best use as few of them as possible.

arielshaqed · 2025-04-30T07:15:07Z

cmd/lakectl/cmd/root.go

@@ -329,7 +329,7 @@ func getSyncArgs(args []string, requireRemote bool, considerGitRoot bool) (remot
 		gitRoot, err := git.GetRepositoryPath(localPath)
 		if err == nil {
 			localPath = gitRoot
-		} else if !(errors.Is(err, giterror.ErrNotARepository) || errors.Is(err, giterror.ErrNoGit)) { // allow support in environments with no git
+		} else if !errors.Is(err, giterror.ErrNotARepository) && !errors.Is(err, giterror.ErrNoGit) { // allow support in environments with no git


Same here, don't like this lint

arielshaqed · 2025-04-30T07:15:59Z

cmd/lakefs/cmd/setup.go

@@ -87,10 +87,12 @@ var setupCmd = &cobra.Command{
 		if err != nil {
 			fmt.Printf("Setup failed: %s\n", err)
 			os.Exit(1)
+			return


Huh? Isn't os.Exit a noreturn func, like panic and a very few others?

arielshaqed · 2025-04-30T07:17:58Z

esti/esti_utils.go

@@ -668,7 +668,11 @@ func checkFileWasGarbageCollected(t *testing.T, presignedURLs map[string]string,
 	if err != nil {
 		t.Fatalf("%s, expected no error, got err=%s", "Http request to presigned url", err)
 	}
-	defer r.Body.Close()
+	defer func() {
+		if err := r.Body.Close(); err != nil {


This is silly for a read, because Close cannot fail. But if it did, I would want to fail the test.
Can we nolint this here, with some content?

arielshaqed · 2025-04-30T07:22:48Z

esti/s3_gateway_test.go

@@ -1058,13 +1062,8 @@ func TestDeleteObjects(t *testing.T) {
 	ctx, _, repo := setupTest(t)
 	defer tearDownTest(repo)
 	const numOfObjects = 10
-	identifiers := make([]types.ObjectIdentifier, 0, numOfObjects)


Oh wow, actual code analysis for Go. Neat!

arielshaqed · 2025-04-30T07:23:37Z

pkg/actions/kv_task_results_iterator_test.go

@@ -36,7 +36,7 @@ func TestTaskResultsIterator(t *testing.T) {
 		},
 		{
 			name:  "after out of range",
-			runID: rand.Intn(100),
+			runID: rand.Intn(100), //nolint:gosec


Can we disable gosec in tests? Preferably just the "pseudorandom numbers" thing?

arielshaqed · 2025-04-30T07:24:03Z

pkg/actions/lua/storage/gcloud/gcs.go

@@ -99,7 +99,7 @@ func writeFuseSymlink(c *GSClient) lua.Function {
 		}

 		w := obj.NewWriter(c.ctx)
-		w.ObjectAttrs.Metadata = map[string]string{
+		w.Metadata = map[string]string{


arielshaqed · 2025-04-30T07:24:37Z

pkg/actions/service_test.go

@@ -34,12 +34,12 @@ const (
 )

 type ActionStatsMockCollector struct {
-	Hits map[string]int
+	Hits map[string]uint64


What? No, I thought the whole idea was to avoid unsigned arithmetic wherever possible.

To quote some people who think they know Go:

When you need an integer value you should use int unless you have a specific reason to use a sized or unsigned integer type.
(My emphasis)

For counters and sizes signed integers are almost always better, they avoid making silly errors such as

var ( counter, max uint64 ) max = max.MaxUint64 - 3 // ... // Check if we have room for 5 more if counter + 5 < max { // Gotcha! If counter == max-1 then counter + 5 == 1 and we're in here. }

arielshaqed · 2025-04-30T08:28:53Z

pkg/api/controller.go

@@ -317,7 +317,8 @@ func (c *Controller) UploadPart(w http.ResponseWriter, r *http.Request, body api

 func (c *Controller) UploadPartCopy(w http.ResponseWriter, r *http.Request,
 	body apigen.UploadPartCopyJSONRequestBody, dstRepository string, branch string, uploadID string, partNumber int,
-	params apigen.UploadPartCopyParams) {
+	params apigen.UploadPartCopyParams,


If different formatting is now required then gofmt should enforce it.

upgrade golangci-lint to v2

528886a

handle some of the lint errors next PR will enable more on test code

nopcoder added area/testing Improvements or additions to tests infrastructure build, deploy and release processes labels Apr 27, 2025

nopcoder self-assigned this Apr 27, 2025

revert interface spelling fix

dab1296

remove golangci-lint output format

80c4ab5

no longer supported

nopcoder added the exclude-changelog PR description should not be included in next release changelog label Apr 27, 2025

nopcoder requested a review from a team April 27, 2025 08:49

nopcoder added the minor-change Used for PRs that don't require issue attached label Apr 27, 2025

arielshaqed requested changes Apr 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade golangci-lint to v2 #8986

Upgrade golangci-lint to v2 #8986

nopcoder commented Apr 27, 2025 •

edited

Loading

github-actions bot commented Apr 27, 2025

github-actions bot commented Apr 27, 2025

arielshaqed left a comment

arielshaqed Apr 30, 2025

arielshaqed Apr 30, 2025

arielshaqed Apr 30, 2025

arielshaqed Apr 30, 2025

arielshaqed Apr 30, 2025

arielshaqed Apr 30, 2025

arielshaqed Apr 30, 2025

arielshaqed Apr 30, 2025

arielshaqed Apr 30, 2025

arielshaqed Apr 30, 2025

arielshaqed Apr 30, 2025

Upgrade golangci-lint to v2 #8986

Are you sure you want to change the base?

Upgrade golangci-lint to v2 #8986

Conversation

nopcoder commented Apr 27, 2025 • edited Loading

github-actions bot commented Apr 27, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Apr 27, 2025

E2E Test Results - Quickstart

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder commented Apr 27, 2025 •

edited

Loading