Skip to content
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

🌱 Enable excluded lint checks #4624

Merged
merged 1 commit into from
May 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,21 @@ linters:
issues:
Copy link
Member

@sbueringer sbueringer May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional ideas for linters:

ref: https://golangci-lint.run/usage/linters/

Just some ideas. I don't want to start a lengthy discussion like tab vs spaces :). But if we get consensus on one / a few of them, that would be nice.

If we want them, they should probably be added commented out here and then introduced one at a time (like with the exclude below)

Copy link
Member

@vincepri vincepri May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on exportloopref, ifshort, and nilerr

The others require new tooling that are more stylistic than not, which I'm generally fine to leave as is

max-same-issues: 0
max-issues-per-linter: 0
# We are disabling default golangci exclusions because we want to help reviewers to focus on reviewing the most relevant
# changes in PRs and avoid nitpicking.
exclude-use-default: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding a comment here:

We are disabling default golangci exclusions because we want to help reviewers to focus on reviewing the most relevant changes in PRs and not too much on nitpicking.

# List of regexps of issue texts to exclude, empty list by default.
exclude:
- Using the variable on range scope `(tc)|(rt)|(tt)|(test)|(testcase)|(testCase)` in function literal
- "G108: Profiling endpoint is automatically exposed on /debug/pprof"
- Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
Copy link
Member

@sbueringer sbueringer May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we want to get rid of a few of them. not the ones about fmt.Fprintf, but e.g. File.Close()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, file.Close() in a defer is actually useful. While ignoring the error can be odd, if a file cannot be closed (because it's a syscall) an error is usually thrown way earlier.

Copy link
Member

@sbueringer sbueringer May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, missed your comment above. Then it's fine for me.

Just had a few of those when close failed when writing to S3/GCS and it's pretty hard to debug. But we also have "manual" reviews, so we should be good

# The following are being worked on to remove their exclusion. This list should be reduced or go away all together over time.
# If it is decided they will not be addressed they should be moved above this comment.
- (comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stmcginnis That's the one which would detect these kinds of error right? https://github.com/kubernetes-sigs/cluster-api/pull/4607/files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I believe so. Next in the list to tackle. ;)

- package comment should be of the form "Package (.+) ..."
stmcginnis marked this conversation as resolved.
Show resolved Hide resolved
- Subprocess launch(ed with variable|ing should be audited)
- (Expect directory permissions to be 0750 or less|Expect file permissions to be 0600 or less)
- (G104|G307)
vincepri marked this conversation as resolved.
Show resolved Hide resolved

run:
timeout: 10m
Expand Down
2 changes: 1 addition & 1 deletion test/framework/alltypes_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func dumpObject(resource runtime.Object, logPath string) {
namespace := metaObj.GetNamespace()
name := metaObj.GetName()

resourceFilePath := path.Join(logPath, namespace, kind, name+".yaml")
resourceFilePath := filepath.Clean(path.Join(logPath, namespace, kind, name+".yaml"))
Expect(os.MkdirAll(filepath.Dir(resourceFilePath), 0755)).To(Succeed(), "Failed to create folder %s", filepath.Dir(resourceFilePath))

f, err := os.OpenFile(resourceFilePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
Expand Down
2 changes: 1 addition & 1 deletion test/framework/bootstrap/kind_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ func save(ctx context.Context, image, dest string) error {
// copied from kind https://github.com/kubernetes-sigs/kind/blob/v0.7.0/pkg/cmd/kind/load/docker-image/docker-image.go#L158
// loads an image tarball onto a node.
func load(imageTarName string, node kindnodes.Node) error {
f, err := os.Open(imageTarName)
f, err := os.Open(filepath.Clean(imageTarName))
if err != nil {
return errors.Wrap(err, "failed to open image")
}
Expand Down
2 changes: 1 addition & 1 deletion test/framework/deployment_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func WatchDeploymentLogs(ctx context.Context, input WatchDeploymentLogsInput) {
go func(pod corev1.Pod, container corev1.Container) {
defer GinkgoRecover()

logFile := path.Join(input.LogPath, input.Deployment.Name, pod.Name, container.Name+".log")
logFile := filepath.Clean(path.Join(input.LogPath, input.Deployment.Name, pod.Name, container.Name+".log"))
Expect(os.MkdirAll(filepath.Dir(logFile), 0755)).To(Succeed())

f, err := os.OpenFile(logFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
Expand Down
3 changes: 2 additions & 1 deletion test/framework/kubetest/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,15 @@ import (
"io"
"os"
"path"
"path/filepath"
)

func copyFile(srcFilePath, destFilePath string) error {
err := os.MkdirAll(path.Dir(destFilePath), 0o750)
if err != nil {
return err
}
srcFile, err := os.Open(srcFilePath)
srcFile, err := os.Open(filepath.Clean(srcFilePath))
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion test/framework/namespace_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func WatchNamespaceEvents(ctx context.Context, input WatchNamespaceEventsInput)
Expect(input.ClientSet).NotTo(BeNil(), "input.ClientSet is required for WatchNamespaceEvents")
Expect(input.Name).NotTo(BeEmpty(), "input.Name is required for WatchNamespaceEvents")

logFile := path.Join(input.LogFolder, "resources", input.Name, "events.log")
logFile := filepath.Clean(path.Join(input.LogFolder, "resources", input.Name, "events.log"))
Expect(os.MkdirAll(filepath.Dir(logFile), 0755)).To(Succeed())

f, err := os.OpenFile(logFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
Expand Down
2 changes: 1 addition & 1 deletion test/infrastructure/docker/docker/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ func (m *Machine) PreloadLoadImages(ctx context.Context, images []string) error
defer os.RemoveAll(dir)

for i, image := range images {
imageTarPath := filepath.Join(dir, fmt.Sprintf("image-%d.tar", i))
imageTarPath := filepath.Clean(filepath.Join(dir, fmt.Sprintf("image-%d.tar", i)))

err = exec.Command("docker", "save", "-o", imageTarPath, image).Run()
if err != nil {
Expand Down