Add staticcheck github action, resolve existing lint #254
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
==========================================
+ Coverage 73.74% 73.80% +0.06%
==========================================
Files 25 25
Lines 1550 1550
==========================================
+ Hits 1143 1144 +1
+ Misses 297 295 -2
- Partials 110 111 +1 ☔ View full report in Codecov by Sentry. |
|
Looks like one of the tests caught a data race somewhere, I'm assuming it's unrelated to this PR. I can probably fix it and make another PR for it. |
| @@ -79,19 +78,18 @@ func CreateServerTLSConfig(caCertPath, serverCertPath, serverKeyPath string) (*t | |||
| InsecureSkipVerify: true, | |||
| ClientCAs: caCertPool, | |||
| } | |||
| tlsConfig.BuildNameToCertificate() | |||
There was a problem hiding this comment.
Reading the deprecation notice this seems to be fine, but I am not entirely sure since the notice is a bit vague.
Are you able to verify that the example server that uses this functionality still works fine? The clients should be able to connect if everything is working correctly.
There was a problem hiding this comment.
Sure, I can add a test with a TLS server.
There was a problem hiding this comment.
Thoughts on using #202?
There was a problem hiding this comment.
Yeah #202 will need to be updated, it has conflicts. I don't think you need to add a test as part of this particular PR. If you can confirm by running the examples manually that they still work we should be good and can merge this PR.
There was a problem hiding this comment.
make run-examples should build and run the example server and agent and if you go to http://localhost:4321/ you should be able to see a connected agent.
Click on it, scroll down and ensure you see it connected using a client certificate, e.g.:
There was a problem hiding this comment.
Verified that the client certificate is shown on this page. Thanks for the instructions!
|
As an aside, I tried to fix the data race in the example, but architecturally it's actually a bit difficult to do. The Wait() on the command is causing a race when other bits of the system write, and it won't be a simple fix unfortunately. Just thought I'd mention my findings. |
Can you please create an issue and describe your findings so that we can fix it later? |
|
Filed #256 |
|
Thank you for the PR @echlebek ! |
This commit adds staticcheck as a github action and resolves existing issues flagged by staticcheck. I've grouped the issues into individual commits so that they can be considered independently. Most of the issues are deprecated uses of stdlib, unused errors, and ineffective branches or statements. But, there is one stylistic issue too that is more subjective.
staticcheck is a very conservative linter that has a focus on minimizing false positives. I've found that staticcheck has improved the quality of my own work, and has also made pull requests easier to evaluate.
Each issue is tagged with an identifier that can be consulted here: https://staticcheck.dev/docs/checks/