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 GolangCI Lint #104

Merged
merged 10 commits into from
Jan 13, 2022
Merged

Add GolangCI Lint #104

merged 10 commits into from
Jan 13, 2022

Conversation

kensipe
Copy link
Contributor

@kensipe kensipe commented Jan 11, 2022

  • Adding GolangCI Lint for linting
  • Added lint task to Makefile
  • Resolved basic linting issues

Lint Resolution

  • Removed kubernetesVersion as it is private and used. If desired, please indicate how it is to be used
  • time.Since is preferred to time.Now().Sub otherwise same functionality
  • Likely the most controversial, The assignment of downloadURL := u was never used. if err not nil, it is assigned to u otherwise an error is returned. It is better and more readable in Go to not use else unless required, in this case, it is much more clear.

Looking forward to adding more linting rules. I was looking for feedback on this first.

Signed-off-by: Ken Sipe <kensipe@gmail.com

@kensipe
Copy link
Contributor Author

kensipe commented Jan 12, 2022

@abiosoft any thoughts? or help regarding Linting?

@abiosoft
Copy link
Owner

@kensipe I was working on the codebase at the time and would rather not deal with conflicts.
You can rebase now and I'll have a look afterwards.

Thanks.

Signed-off-by: Ken Sipe <kensipe@gmail.com>
Signed-off-by: Ken Sipe <kensipe@gmail.com>
Signed-off-by: Ken Sipe <kensipe@gmail.com>
Signed-off-by: Ken Sipe <kensipe@gmail.com>
Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe
Copy link
Contributor Author

kensipe commented Jan 13, 2022

@abiosoft rebase complete.. no merge conflicts to resolve

@kensipe
Copy link
Contributor Author

kensipe commented Jan 13, 2022

it's worth noting but perhaps obvious... the first lint requires connectivity to pull the versioned linter. All other lints can work with a disconnected machine.

Makefile Outdated Show resolved Hide resolved
This reverts commit 06e60e8.
Signed-off-by: Ken Sipe <kensipe@gmail.com>
…e for now

Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe
Copy link
Contributor Author

kensipe commented Jan 13, 2022

@abiosoft ok. Add adjustments made. Worth noting that this is the initial stage. The config file is empty making this PR small. I was planning on adding a number of linters for your review after this is merged.

As noted, I still have concerns over linter versioning but happy to seeing linting

As note that based on guidance, there is a total assumption the linter is in the path. The result of it not being in the path is:

make lint
golangci-lint --timeout 3m run
make: golangci-lint: No such file or directory
make: *** [lint] Error 1

@abiosoft
Copy link
Owner

One last thing to make this actually be part of the CI, can you add a lint step as part of the build job in .github/workflows/go.yml?

@abiosoft
Copy link
Owner

As note that based on guidance, there is a total assumption the linter is in the path. The result of it not being in the path is:

make lint
golangci-lint --timeout 3m run
make: golangci-lint: No such file or directory
make: *** [lint] Error 1

Not an issue, it will be available in the CI environment. And anyone that wants to run it locally can always install it.

Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe
Copy link
Contributor Author

kensipe commented Jan 13, 2022

@abiosoft

.github/workflows/go.yml Outdated Show resolved Hide resolved
Signed-off-by: Ken Sipe <kensipe@gmail.com>
@abiosoft abiosoft merged commit 476d526 into abiosoft:main Jan 13, 2022
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.

2 participants