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

*: fix staticcheck lint #16626

Merged
merged 4 commits into from
Sep 21, 2023
Merged

*: fix staticcheck lint #16626

merged 4 commits into from
Sep 21, 2023

Conversation

fuweid
Copy link
Member

@fuweid fuweid commented Sep 21, 2023

Changed TraceKey/StartTimeKey/TokenFieldNameGRPCKey to struct{} to follow the correct usage of context. Similar patch to #8901.

And this patch also copies the tools/.golangci.yaml and run the linters for which we have already fixed.
The temp .golangci.yaml will be removed when we fixes all the linters' issues.

Added lint_pass in scripts/test.sh to run the golangci linters for each modules. And disable no working linters in the global golangci.yaml and enable them by #16610.

Part of #16610

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

Changed TraceKey/StartTimeKey/TokenFieldNameGRPCKey to struct{} to
follow the correct usage of context. Similar patch to etcd-io#8901.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
Copy the tools/.golangci.yaml and run the linters for which we have
already fixed. The temp .golangci.yaml will be removed when we fixes all
the linters' issues.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid fuweid marked this pull request as ready for review September 21, 2023 05:28
scripts/test.sh Outdated Show resolved Hide resolved
Makefile Outdated
.PHONY: verify-ineffassign
verify-ineffassign:
PASSES="ineffassign" ./scripts/test.sh
.PHONY: verify-golangcilint
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep one target for linting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Makefile Outdated Show resolved Hide resolved
Disable failed linters and enable it by etcd-io#16610.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid
Copy link
Member Author

fuweid commented Sep 21, 2023

ping @ahrtr ~

@@ -169,7 +169,6 @@ func loadKeyValueOperations(path string) (operations []porcupine.Operation, err
func persistWatchOperations(t *testing.T, lg *zap.Logger, path string, responses []model.WatchOperation) {
lg.Info("Saving watch operations", zap.String("path", path))
file, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0755)
defer file.Close()
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

Comment on lines 74 to 76
if err = b.myKey.Delete(); err != nil {
// Nothing to do here. We have to wait for the key to be
// deleted when the lease expires.
}
b.myKey.Delete()
// Nothing to do here even if we run into error.
// We have to wait for the key to be deleted when the lease expires.
Copy link
Member

@ahrtr ahrtr Sep 21, 2023

Choose a reason for hiding this comment

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

Suggest to keep this unchanged. Previous source code explicitly comments why the error is ignored.

It can't even pass the static check, because the returned error should be explicitly ignored (i.e. _ = xxxx) or be checked. The reason why all the workflows are green is probably because some checks/lints are disabled?

Copy link
Member Author

@fuweid fuweid Sep 21, 2023

Choose a reason for hiding this comment

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

It can't even pass the static check, because all returned errors should be explicitly ignored (i.e. _ = xxxx) or be checked. The reason why all the workflows are green is probably because some checks/lints are disabled?

we don't enable errcheck before. So it can pass the check.

Suggest to keep this unchanged. Previous code explicitly commenting why the error is ignored.

Sounds good. I was thinking that it's good enough to keep the comment. I updated with disable linter comment. Please take a look.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

nice work! thx @fuweid

@ahrtr ahrtr merged commit 9079ab3 into etcd-io:main Sep 21, 2023
27 checks passed
@fuweid fuweid deleted the fix-staticcheck-lint branch September 21, 2023 10:17
@jmhbnz jmhbnz mentioned this pull request Sep 25, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants