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

chore/linting #24

Merged
merged 11 commits into from
Dec 6, 2023
Merged

chore/linting #24

merged 11 commits into from
Dec 6, 2023

Conversation

puffitos
Copy link
Collaborator

Motivation

Close #23

Changes

Added linting with golangci, turned off some of the most strict linters. Also implemented most of the linter's suggestions. We should discuss some of the TODOs before merging (a lot of errors were left unchecked...)

Tests done

TODO

  • I've assigned this PR to myself
  • I've labeled this PR correctly

…gestions\n- RoutingTree is now a pointer to avoid mutex value copy\n- fixed some tests\n- added constants\n- simplified some loops

Signed-off-by: Bruno Bressi <[email protected]>
- linted files based on suggestions
- RoutingTree is now a pointer to avoid mutex value copy
- fixed some tests
- added constants
- simplified some loops

Signed-off-by: Bruno Bressi <[email protected]>
@puffitos puffitos added the feature Introduces a new feature label Nov 29, 2023
@puffitos puffitos self-assigned this Nov 29, 2023
@puffitos puffitos changed the title Chore/linting [WIP] Chore/linting Nov 29, 2023
Copy link
Collaborator

@lvlcn-t lvlcn-t left a comment

Choose a reason for hiding this comment

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

Some minor things that can be improved

pkg/checks/health.go Show resolved Hide resolved
pkg/checks/health.go Show resolved Hide resolved
pkg/checks/health.go Outdated Show resolved Hide resolved
pkg/config/http.go Outdated Show resolved Hide resolved
pkg/sparrow/run.go Outdated Show resolved Hide resolved
@lvlcn-t
Copy link
Collaborator

lvlcn-t commented Dec 5, 2023

Additionally we should talk about/take care of the many open TODO comments that are in our code base.

Signed-off-by: Bruno Bressi <[email protected]>
@puffitos puffitos requested a review from eumel8 as a code owner December 6, 2023 10:57
@puffitos puffitos changed the title [WIP] Chore/linting chore/linting Dec 6, 2023
@puffitos
Copy link
Collaborator Author

puffitos commented Dec 6, 2023

PR ready after call with developers. The tests will fail (errors unhandled and unittests). Those are handled in a separate PR.

@puffitos puffitos requested a review from lvlcn-t December 6, 2023 11:15
@lvlcn-t lvlcn-t added refactoring Refactoring of existing code housekeeping and removed feature Introduces a new feature labels Dec 6, 2023
Copy link
Collaborator

@lvlcn-t lvlcn-t left a comment

Choose a reason for hiding this comment

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

LGTM overall, just had some minor questions/notes.

internal/helper/retry.go Show resolved Hide resolved
pkg/config/http.go Show resolved Hide resolved
pkg/sparrow/run.go Show resolved Hide resolved
@y-eight
Copy link
Member

y-eight commented Dec 6, 2023

I have just added 2 small things: Indent for json response of checks metrics endpoint; Reduced interval of health check to 15 sec. This will be configurable in the future by #34

PR is LGTM

@puffitos puffitos merged commit 2e90825 into main Dec 6, 2023
3 of 6 checks passed
@lvlcn-t lvlcn-t deleted the chore/linting branch December 7, 2023 12:16
@lvlcn-t lvlcn-t mentioned this pull request Dec 8, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping refactoring Refactoring of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI sec check fail & no linting is present
3 participants