-
Notifications
You must be signed in to change notification settings - Fork 4.6k
vet: run staticcheck for all sub modules #7155
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
Conversation
|
Verified the behavior of |
| }` | ||
|
|
||
| cleanup, err := createTmpConfigInFileSystem(configJSON) | ||
| cleanup, _ := createTmpConfigInFileSystem(configJSON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.Fatal on err instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
scripts/vet.sh
Outdated
| # - Do not import x/net/context. | ||
| not git grep -l 'x/net/context' -- "*.go" | ||
| not git grep --quiet -l 'x/net/context' -- "*.go" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just remove this? It's a symlink now, and there's no reason we would be using it anymore, anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'm not so sure about it. Removing it from vet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did I say "symlink"? I meant "type alias" O_O
| pushd ${MOD_DIR} | ||
| # - Collection of static analysis checks | ||
| SC_OUT="$(mktemp)" | ||
| staticcheck -go 1.19 -checks 'all' ./... >"${SC_OUT}" || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just move this into the previous for loop instead of adding another one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
scripts/vet.sh
Outdated
| # Error for anything other than checks that need exclusions. | ||
| grep -v "(ST1000)" "${SC_OUT}" | grep -v "(SA1019)" | grep -v "(ST1003)" | not grep -v "(ST1019)\|\(other import of\)" | ||
| # Error for anything other than checks that need exclusions. | ||
| (grep -v "(ST1000)" "${SC_OUT}" || : )| (grep -v "(SA1019)" || :) | (grep -v "(ST1003)" || :) | (grep -v "(ST1019)\|\(other import of\)" || :) | not grep -v "(SA4000)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define a function for this grep || : pattern instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
security/advancedtls/crl.go
Outdated
| } | ||
| der := cryptobyte.String(crlBytes) | ||
| var issuer cryptobyte.String | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? was this a linter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
+@gtcooke94 to review the advancedtls changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, very small changes in advancedtls
|| :to grep. This is because grep returns error when there are no lines found.Basically, fixes #7141
RELEASE NOTES: none