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

Formatter, Linter and fixes #62

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Formatter, Linter and fixes #62

wants to merge 5 commits into from

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Jan 19, 2024

See #49
Fixes issues found by a gosec run

  • Introduces "request-header-timeout" param

cmd/lingo/main.go Show resolved Hide resolved
@alpe alpe changed the title Fix issues reported by gosec Formatter, Linter and fixes Jan 19, 2024
Makefile Outdated Show resolved Hide resolved
pkg/deployments/scaler.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@nstogner nstogner left a comment

Choose a reason for hiding this comment

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

A useful PR. Thanks for submitting! Added some comments.

cmd/lingo/main.go Outdated Show resolved Hide resolved
pkg/autoscaler/autoscaler.go Outdated Show resolved Hide resolved
pkg/proxy/handler.go Show resolved Hide resolved
pkg/queue/queue.go Show resolved Hide resolved
Copy link
Contributor

@samos123 samos123 left a comment

Choose a reason for hiding this comment

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

LGTM, but unsure about the RequestHeaderTimeout.

Makefile Outdated Show resolved Hide resolved
pkg/autoscaler/autoscaler.go Outdated Show resolved Hide resolved
cmd/lingo/main.go Show resolved Hide resolved
.golangci.yaml Outdated

linters:
disable-all: true
enable:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to take a minimal approach to linting here. IMO too many linters leads to an annoying development experience and excessive comments to appease all of the linters. Can we stick to the official go toolchain?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a tool that should absolutely be added in excess of the standard toolchain, can we add a comment as to why it is needed and is not redundant?

@alpe
Copy link
Contributor Author

alpe commented Feb 13, 2024

I have removed the linter config and integration to move this forward. Personally, I have seen a lot of benefits in linters in addition to the Go toolchain but I am definitively not dogmatic about it. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants