Closed ProofOfKeags closed 2 months ago
[!IMPORTANT]
Auto Review Skipped
Auto reviews are disabled on this repository.
Please check the settings in the CodeRabbit UI or the
.coderabbit.yaml
file in this repository. To trigger a single review, invoke the@coderabbitai review
command.You can disable this status message by setting the
reviews.review_status
tofalse
in the CodeRabbit configuration file.
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
No strong opinion on this, as I didn't encounter these warnings anywhere so far.
Here are my two cents about the other dependencies:
- andybalholm/brotli --> only used in a single place, can perhaps add a
//nolint
directive there?- davecgh/go-spew/spew --> yes, makes sense to bless
- decred/dcrd/dcrec/secp256k1/v4 --> yes, makes sense to bless
- go-errors/errors --> should not be used anymore, can use the
errors
package from stdlib- jessevdk/go-flags --> can conditionally bless for any files under
cmd/lnd
?- stretchr/testify/assert --> can conditionally bless for any
*_test.go
files? Same for those below.- stretchr/testify/mock
- stretchr/testify/require
- urfave/cli --> can conditionally bless for any files under
cmd/lncli
?
Agree with Oli here, I also have no super strong opinion. Maybe one issue with the depguard is that now for any added dependency we'd need to modify the yaml too besides just the go.mod. Other than that LGTM!
which version is this linter? Tried golangci-lint run
locally and saw nothing new...
@yyforyongyu
which version is this linter? Tried golangci-lint run locally and saw nothing new...
golangci-lint has version v1.55.2 built with go1.21.6 from (unknown, mod sum: "h1:yllEIsSJ7MtlDBwDJ9IMBkyEUz2fYE0b5B8IUgO1oP8=") on (unknown)
No strong opinion on this, as I didn't encounter these warnings anywhere so far.
Here are my two cents about the other dependencies:
- andybalholm/brotli --> only used in a single place, can perhaps add a
//nolint
directive there?- davecgh/go-spew/spew --> yes, makes sense to bless
- decred/dcrd/dcrec/secp256k1/v4 --> yes, makes sense to bless
- go-errors/errors --> should not be used anymore, can use the
errors
package from stdlib- jessevdk/go-flags --> can conditionally bless for any files under
cmd/lnd
?- stretchr/testify/assert --> can conditionally bless for any
*_test.go
files? Same for those below.- stretchr/testify/mock
- stretchr/testify/require
- urfave/cli --> can conditionally bless for any files under
cmd/lncli
?
ACK on all I'll incorporate this.
Agree with Oli here, I also have no super strong opinion. Maybe one issue with the depguard is that now for any added dependency we'd need to modify the yaml too besides just the go.mod. Other than that LGTM!
I'm willing to disable depguard entirely! I was proceeding under the assumption that it is wanted. I'd imagine @Crypt-iQ might have something to say about this.
Ok right now it looks like conditional blessing is really really painful so I'm going to just not. I'm hoping that's OK for the time being.
Ok looks like I fixed my own problem by just pinning golangci-lint to 1.52.2 which is what is in lnd-tools.
Change Description
This updates our CI lint config for depguard to reduce noise. The initial commit includes explicit blessings for obviously wanted dependencies:
After explicitly permitting these package hierarchies, the following dependencies are still considered sus:
I'm soliciting opinions from the LL elder wizards about whether we would like to bless these as well, or remove them.
My initial inclination is to add all of the
stretchr/testify
packages to the list as well since we intentionally make significant use of them. The decred dependency seems especially sus to me given that it has potentially adversarial incentives and is including cryptography implementations. The rest I'm unsure.Steps to Test
$ golangci-lint run
both before and after this commit series.Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.