Create linter configuration and address found issues #10

rdner · 2022-02-16T09:41:23Z

This PR is to propose the linting configuration that we are going to re-use for new repositories.

Also, all the currently found linting issues are fixed in this PR which can give an idea what kind of common issues we have now in our code and which the linter finds.

Closes elastic/beats#30150

logp/config.go

logp/core.go

elasticmachine · 2022-02-16T09:55:51Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-02-17T10:16:01.240+0000
Duration: 15 min 42 sec

Test stats 🧪

Test	Results
Failed	0
Passed	124
Skipped	0
Total	124

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

.ci/Jenkinsfile

.golangci.yml

ph · 2022-02-16T15:15:15Z

.golangci.yml

+
+  prealloc:
+    # XXX: we don't recommend using this linter before doing performance profiling.
+    # For most programs usage of prealloc will be a premature optimization.


@rdner Looking at the comment we might want to remove it?

Every comment was just copied from the official doc. And it makes sense to me. By default, we should pre-allocate but it does not mean that it's required.

https://golangci-lint.run/usage/linters/#prealloc

I'm not a huge fan of this one either, I'm not a huge fan of premature optimisations. I'm just wondering if it can get noisy.

I believe if it's known one should always set capacity when allocating, for example:

b := map[string]interface{}{} a := make([]string, 0, len(b)) // vs `var a []string` for key := range b { a = append(a, key) }

I removed the comment that it would not scare anyone. I'd like to keep the linter though unless everyone are oppose it.

I don't mind this, it's not an optimization that makes the code harder to read. If anything it makes the allocation more obvious.

I have seen preallocating like this make a big performance difference in frequently used code that creates small slices/arrays. The underlying array doubles when there's not enough capacity, which happens more frequently if the slice was empty and stays small (per "Growing Slices" in https://go.dev/blog/slices-intro)

ph · 2022-02-16T15:16:46Z

.golangci.yml

+    # Enable to require nolint directives to mention the specific linter being suppressed. Default is false.
+    require-specific: true
+
+  staticcheck:


NIT: It would be nice to have the 1.17 to be populated by the .go-version file, could be done with the mage task.

Well, that would mean we make a template configuration that we build on CI right before we run the linter.

These versions are configurations for built-in linters, so every built-in linter (plugin) is free to choose how they configure themselves and they don't seem to support .go-version.

In the long run it would be nice to have it reading from .go-version so we don't need to remember to update this configuration file as we change our Go version.

Because we're most likely running it with mage, I see no issue on adding this automation.

@rdner This shouldn't be a lot of effort, we have already code generation. There is a make update task that generates the configuration files from templates and other generated files. That make update comment is run on every build, if git status detects new files or modified files it will fail the build.

So the workflow would be like this.

Update .go-version

run make update

Push PR with the updated linter information and the updated go version.

We have an automated task that does this [automation] Update go release version 1.17.7 fleet-server#1143

But on the other side we will have 1.17 for some time and 1.18 for quite some time, I just hate having multiple files to updates we still have too much and removing the human in the operation is always a good thing and having multiple go repository that just work the same would save us time.

@ph This PR already has a ton of comments, I'm not going to implement it here but I can take it as my next step.

ph

Change looks to me, the number of linter feels a bit overwhelming, adding a comment to remove the prealloc since we haven't done any performance analysis, yet and we with the architecture change it might be premature to worry about it. If possible we should remove the 1.17 magic number from that files, we can populate it from the .go-version file.

To reduce the noise and make the run faster, it would be nice in ci if the check was only run on the added or modified files. This would allow us to enforce it from day 1, and incrementally improve the code as we go.

So far, I would be +1 to share the same across all of our repositories.

belimawr

Great job @rdner !!

One little detail, once the PR is finalised, what about sorting the liners alphabetically? I feel it makes reading easier.

.ci/Jenkinsfile

.golangci.yml

logp/core.go

logp/core_test.go

logp/global_test.go

logp/selective_test.go

paths/paths.go

.golangci.yml

cmacknz · 2022-02-16T19:00:09Z

Thanks for proposing this @rdner. There is a lot to deal with to get this enabled it seems.

rdner · 2022-02-17T10:20:15Z

.ci/Jenkinsfile

-              sh(label: 'lint', script: '''
-                go mod tidy && git diff --exit-code
-                gofmt -l . | read && echo "Code differs from gofmt's style. Run 'gofmt -w .'" 1>&2 && exit 1 || true
-              ''')


go mod tidy has been moved to the magefile

gofmt is now included into the new linter

rdner · 2022-02-17T10:20:27Z

.ci/Jenkinsfile

              sh(label: 'Mage check', script: 'mage check')
-              sh(label: 'Go vet', script: 'go vet ./...')


Is now included into the new linter.

rdner · 2022-02-17T10:26:05Z

file/rotator.go

@@ -380,7 +384,12 @@ func (r *Rotator) closeFile() error {
 	}
 	err := r.file.Close()
 	r.file = nil
-	return errors.Wrap(err, "failed to close active file")


Wrap returns nil if the err == nil, fmt.Errorf does not, so had to adjust.

rdner · 2022-02-17T10:30:17Z

Since this PR is already quite big, I'm going to create a follow-up PR where I will add support for:

.go-version file and templating the linting configuration
mage LintLastCommit which will lint only Go-files affected by the last commit (which will become the default on our CI).

faec

🎉

belimawr

I know it has been merged, but I'm still submitting it so I don't lose all I've written :P

logp/environment.go

magefile.go

Create linter configuration and address found issues

b0837c3

rdner added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Feb 16, 2022

rdner self-assigned this Feb 16, 2022

rdner requested review from kvch, belimawr, cmacknz, faec and fearful-symmetry February 16, 2022 09:41

rdner commented Feb 16, 2022

View reviewed changes

logp/config.go Show resolved Hide resolved

rdner commented Feb 16, 2022

View reviewed changes

logp/core.go Show resolved Hide resolved

Add the linter to Jenkins

97bd94a

kvch reviewed Feb 16, 2022

View reviewed changes

.ci/Jenkinsfile Outdated Show resolved Hide resolved

ph reviewed Feb 16, 2022

View reviewed changes

.golangci.yml Outdated Show resolved Hide resolved

ph reviewed Feb 16, 2022

View reviewed changes

ph approved these changes Feb 16, 2022

View reviewed changes

belimawr reviewed Feb 16, 2022

View reviewed changes

rdner added 3 commits February 16, 2022 18:10

Use neutral English for spell-checking

357a34a

Clean up the config, remove redundant linters

f246a39

Address PR comments about error checks

efb8680

cmacknz reviewed Feb 16, 2022

View reviewed changes

.golangci.yml Outdated Show resolved Hide resolved

cmacknz reviewed Feb 16, 2022

View reviewed changes

.golangci.yml Outdated Show resolved Hide resolved

rdner added 6 commits February 17, 2022 09:29

Replace the custom errors package with the standard library

af1f8c3

Fix broken error returns

8c81821

Remove SQL linters

5b9bf5d

Enable the constant linter and fix issues

ec816de

Fix import on Windows

53ceaba

Move linting to the magefile

af131c8

rdner added 2 commits February 17, 2022 11:02

Fix the "file is busy" error

a29efde

Make the forbidigo comment clearer

bc2c107

rdner commented Feb 17, 2022

View reviewed changes

rdner requested review from ph, cmacknz, kvch and belimawr February 17, 2022 10:31

ph approved these changes Feb 17, 2022

View reviewed changes

kvch approved these changes Feb 17, 2022

View reviewed changes

cmacknz approved these changes Feb 17, 2022

View reviewed changes

faec approved these changes Feb 17, 2022

View reviewed changes

rdner merged commit cb28916 into elastic:main Feb 17, 2022

rdner deleted the add-linter branch February 17, 2022 16:05

belimawr reviewed Feb 17, 2022

View reviewed changes

logp/environment.go Show resolved Hide resolved

magefile.go Show resolved Hide resolved

magefile.go Show resolved Hide resolved

rdner mentioned this pull request Feb 18, 2022

Reorganise the magefile #11

Merged

cmacknz mentioned this pull request Feb 22, 2022

{,x-pack/}filebeat: clean up dead code elastic/beats#30483

Merged

3 tasks

This was referenced Feb 23, 2022

Add .go-version support for the linter configuration #12

Merged

Move common.MapStr and rename it to mapstr.M #13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create linter configuration and address found issues #10

Create linter configuration and address found issues #10

rdner commented Feb 16, 2022 •

edited

Loading

elasticmachine commented Feb 16, 2022 •

edited

Loading

Build stats

Test stats 🧪

ph Feb 16, 2022

rdner Feb 16, 2022

rdner Feb 16, 2022

belimawr Feb 16, 2022

rdner Feb 16, 2022

rdner Feb 16, 2022 •

edited

Loading

cmacknz Feb 17, 2022

ph Feb 16, 2022 •

edited

Loading

rdner Feb 16, 2022

belimawr Feb 16, 2022

ph Feb 16, 2022

rdner Feb 17, 2022 •

edited

Loading

ph left a comment

belimawr left a comment

cmacknz commented Feb 16, 2022

rdner Feb 17, 2022

rdner Feb 17, 2022 •

edited

Loading

rdner Feb 17, 2022

rdner commented Feb 17, 2022

faec left a comment

belimawr left a comment

		sh(label: 'Mage check', script: 'mage check')
		sh(label: 'Go vet', script: 'go vet ./...')

Create linter configuration and address found issues #10

Create linter configuration and address found issues #10

Conversation

rdner commented Feb 16, 2022 • edited Loading

elasticmachine commented Feb 16, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

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

rdner Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph Feb 16, 2022 • edited Loading

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

rdner Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

ph left a comment

Choose a reason for hiding this comment

belimawr left a comment

Choose a reason for hiding this comment

cmacknz commented Feb 16, 2022

Choose a reason for hiding this comment

rdner Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdner commented Feb 17, 2022

faec left a comment

Choose a reason for hiding this comment

belimawr left a comment

Choose a reason for hiding this comment

rdner commented Feb 16, 2022 •

edited

Loading

elasticmachine commented Feb 16, 2022 •

edited

Loading

rdner Feb 16, 2022 •

edited

Loading

ph Feb 16, 2022 •

edited

Loading

rdner Feb 17, 2022 •

edited

Loading

rdner Feb 17, 2022 •

edited

Loading