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 golangci-lint in the unit testing phase, fix all the errors #266

Merged
merged 5 commits into from
Apr 22, 2022

Conversation

burmanm
Copy link
Contributor

@burmanm burmanm commented Feb 3, 2022

What this PR does:
Enables golangci-lint and enforces against errors. Also, to pass this PR.. fix all the errors we had.

Which issue(s) this PR fixes:
Fixes #325

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CHANGELOG.md updated (not required for documentation PRs)
  • CLA Signed: DataStax CLA

@bradfordcp
Copy link
Member

Is there an issue for this PR?

@Miles-Garnsey
Copy link
Member

Quick question: with timeouts set to 10 minutes, should we expect all of the tests suites to take 10 minutes longer to enable all the linting? What's the DX impact (if any)?

I think this ticket is a good idea, just wondering if it will delay GHA test runs even more.

@burmanm
Copy link
Contributor Author

burmanm commented Apr 7, 2022

I have no idea what "DX impact" is (and neither did Google), so I can't answer to that. And no, this adds max of 10 minutes to the unit testing phase (from history it seems 1 minute at most to that phase), so there's no real impact to the testing phase as the e2e tests are the slower one and they all run in parallel.

@bradfordcp No ticket, don't think so (could've been in k8ssandra project). This is more of a code quality improvement.

@Miles-Garnsey
Copy link
Member

I have no idea what "DX impact" is (and neither did Google), so I can't answer to that. And no, this adds max of 10 minutes to the unit testing phase (from history it seems 1 minute at most to that phase), so there's no real impact to the testing phase as the e2e tests are the slower one and they all run in parallel.

@bradfordcp No ticket, don't think so (could've been in k8ssandra project). This is more of a code quality improvement.

DX.

If it is one minute in runtime, this should be a real help. A lot of our commits are a bit dirty due to go fmt/vet making unrelated changes at arbitrary times.

@burmanm
Copy link
Contributor Author

burmanm commented Apr 18, 2022

This shouldn't have large amount of impact to developers. On local machine it caches the results, so it takes ~2-3s to run. It's not very strict on checks, mostly those "missing err handling" type of stuff.

Actually, for runtime it's not even going to increase that at all in the GHA. The tests have been done all the time (you can see from the GHA output in some other PRs), but those results haven't been enforced.

Copy link
Contributor

@jsanda jsanda left a comment

Choose a reason for hiding this comment

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

@burmanm can you please can an issue for this?

Do you think it makes sense to integrate golangci-lint into the Makefile? I think so. If you agree, can you please create a follow up ticket for that as well?

@burmanm
Copy link
Contributor Author

burmanm commented Apr 21, 2022

Added makefile target and ticket

@burmanm burmanm merged commit 484cd1d into k8ssandra:master Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

K8SSAND-1470 ⁃ Add static checking to unit tests in GHA
5 participants