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

Add static analysis GitHub action #67

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

martinkennelly
Copy link
Member

@martinkennelly martinkennelly commented Feb 17, 2021

Add static analysis GitHub action.

Add golangci-lint action with extra linters besides the default set [1].
Add hadolint and shellcheck.

[1] https://golangci-lint.run/usage/linters
Part of issue #68

@zshi-redhat
Copy link
Collaborator

Good work, looks like it captures many "Errors".
@martinkennelly would you want to fix those "Errors" in this PR or a separate one?

@martinkennelly
Copy link
Member Author

Separate. I will start to work on them. I am hoping, we can add these basic actions to all our repos. In the last community meeting, we discussed trialing them in NRI first.
I am currently working on e2e test action as well with kind.

@zshi-redhat
Copy link
Collaborator

Separate. I will start to work on them. I am hoping, we can add these basic actions to all our repos. In the last community meeting, we discussed trialing them in NRI first.
I am currently working on e2e test action as well with kind.

@martinkennelly shall we merge this PR or wait until the errors be fixed.

@martinkennelly
Copy link
Member Author

martinkennelly commented Mar 8, 2021

Separate. I will start to work on them. I am hoping, we can add these basic actions to all our repos. In the last community meeting, we discussed trialing them in NRI first.
I am currently working on e2e test action as well with kind.

@martinkennelly shall we merge this PR or wait until the errors be fixed.

@zshi-redhat review please. @emmakenny is working on this and will submit separate PR.

@martinkennelly
Copy link
Member Author

Forced pushed changes: Add golangci lint config file, created a make target, rebased to master and executed go mod tidy.

martinkennelly and others added 8 commits June 4, 2021 22:39
Add golangci-lint action with extra linters
besides the default set [1].
Add hadolint and shellcheck.

[1] https://golangci-lint.run/usage/linters

Signed-off-by: Kennelly, Martin <[email protected]>
Inclusion of golangci-lint config file allows local lint
execution and Makefile inclusion lowers the barrier for
devs and GH workflow consumption.

Signed-off-by: Kennelly, Martin <[email protected]>
Signed-off-by: Kennelly, Martin <[email protected]>
Previous to this change, a webhook port is configurable
via the webhook binary argument, however, installer
does not have a mechanism to change the webhook svc
port for the webhook. This patch exposes this option
to the user and sets a default as a shared type
between webhook and installer.

Signed-off-by: Kennelly, Martin <[email protected]>
In order, to gain a small performance boost in terms of memory and
cpu cycles, we access the data structure using indexing or
pointers instead of copying data when we loop over large data
structures.

Signed-off-by: Kennelly, Martin <[email protected]>
Golint is archived and unmaintained.

Signed-off-by: Kennelly, Martin <[email protected]>
@martinkennelly
Copy link
Member Author

@zshi-redhat I pushed the fixes here for static analysis. I fixed most and left a few that needs to be addressed in another PR. Please see the commit messages for more details. All the commits address static scan issues but I broke some fixes out into separate commits to make it easier for you to review.

I also rebased this entire patch series ontop of the e2e features tests (#98) for regression testing and got an expected outcome including self hosted test to validate hugepages downward API info feature - output:
https://github.com/martinkennelly/network-resources-injector/runs/2750114201?check_suite_focus=true

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.

3 participants