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

server: remaining errors.Is conversions #18617

Merged

Conversation

redwrasse
Copy link
Contributor

Remaining errors.Is conversions for server module, as part of #18576.

@k8s-ci-robot
Copy link

Hi @redwrasse. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 58.53659% with 17 lines in your changes missing coverage. Please review.

Project coverage is 68.80%. Comparing base (5704c61) to head (c1d71fc).
Report is 8 commits behind head on main.

Current head c1d71fc differs from pull request most recent head 0739142

Please upload reports for the commit 0739142 to get more accurate results.

Files with missing lines Patch % Lines
server/etcdserver/api/v2discovery/discovery.go 60.00% 3 Missing and 1 partial ⚠️
server/etcdserver/api/v3rpc/lease.go 71.42% 0 Missing and 2 partials ⚠️
server/etcdserver/api/v3rpc/watch.go 50.00% 1 Missing and 1 partial ⚠️
client/v3/client.go 0.00% 1 Missing ⚠️
server/etcdserver/api/rafthttp/snapshot_sender.go 0.00% 0 Missing and 1 partial ⚠️
server/etcdserver/api/v3rpc/maintenance.go 0.00% 0 Missing and 1 partial ⚠️
server/proxy/grpcproxy/health.go 0.00% 1 Missing ⚠️
server/proxy/grpcproxy/lease.go 0.00% 0 Missing and 1 partial ⚠️
server/proxy/grpcproxy/maintenance.go 0.00% 1 Missing ⚠️
server/storage/backend/batch_tx.go 0.00% 0 Missing and 1 partial ⚠️
... and 2 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
Files with missing lines Coverage Δ
client/v3/retry_interceptor.go 65.61% <100.00%> (ø)
server/etcdserver/api/rafthttp/pipeline.go 97.40% <100.00%> (ø)
server/etcdserver/api/rafthttp/stream.go 81.81% <100.00%> (ø)
server/etcdserver/api/v3compactor/revision.go 79.31% <100.00%> (ø)
server/etcdserver/api/v3rpc/util.go 51.61% <100.00%> (ø)
server/etcdserver/apply/uber_applier.go 86.17% <100.00%> (ø)
server/lease/leasehttp/http.go 64.18% <100.00%> (+2.02%) ⬆️
client/v3/client.go 84.89% <0.00%> (ø)
server/etcdserver/api/rafthttp/snapshot_sender.go 84.15% <0.00%> (ø)
server/etcdserver/api/v3rpc/maintenance.go 73.49% <0.00%> (ø)
... and 9 more

... and 16 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18617      +/-   ##
==========================================
+ Coverage   68.77%   68.80%   +0.03%     
==========================================
  Files         420      420              
  Lines       35535    35539       +4     
==========================================
+ Hits        24439    24454      +15     
+ Misses       9666     9664       -2     
+ Partials     1430     1421       -9     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5704c61...0739142. Read the comment docs.

@jmhbnz
Copy link
Member

jmhbnz commented Sep 22, 2024

/ok-to-test

@ivanvc
Copy link
Member

ivanvc commented Sep 23, 2024

Hi @redwrasse, thanks for your pull request. I think for this PR, we may be missing the following (but correct me if I'm wrong):

  • server/storage/wal/wal_test.go:856
  • server/storage/storage.go:115
  • server/storage/backend/batch_tx.go:128
  • server/etcdserver/api/v3rpc/lease.go:81
  • server/etcdserver/api/v3rpc/maintenance.go:16
  • server/proxy/grpcproxy/health.go:55
  • server/etcdserver/apply/uber_applier.go:126

@redwrasse
Copy link
Contributor Author

Thanks for flagging @ivanvc . Let me add those in here.

@redwrasse
Copy link
Contributor Author

Added in the additional lines.

@redwrasse redwrasse force-pushed the redwrasse/server/remaining-errors-is branch from f597415 to 500b853 Compare September 24, 2024 01:39
@redwrasse redwrasse force-pushed the redwrasse/server/remaining-errors-is branch from 500b853 to 99b0465 Compare September 24, 2024 03:01
@redwrasse
Copy link
Contributor Author

@ivanc sorry, I messed this one up. I may not get to it until tomorrow, or feel free to make the changes yourself if you get the chance.

@ivanvc
Copy link
Member

ivanvc commented Sep 24, 2024

@ivanc sorry, I messed this one up. I may not get to it until tomorrow, or feel free to make the changes yourself if you get the chance.

No worries, we can wait (I don't think I'm even able to edit or add to a pull request 😃)

@redwrasse
Copy link
Contributor Author

@ivanc, I think I've got all cases of the server module. Could you let me know if you see anymore? Then I'll remove the draft status.
Thanks!

@ivanvc
Copy link
Member

ivanvc commented Sep 27, 2024

Thanks, @redwrasse. It looks like you updated all the cases. Please remove the draft so we can see the CI result.

@redwrasse redwrasse marked this pull request as ready for review September 28, 2024 05:29
@ivanvc
Copy link
Member

ivanvc commented Sep 29, 2024

@redwrasse, thanks for updating the pull request. With all the checks passing, can you squash your commits? 🙏

Thanks!

@redwrasse redwrasse force-pushed the redwrasse/server/remaining-errors-is branch from eaac0fc to 0739142 Compare September 29, 2024 12:25
@redwrasse
Copy link
Contributor Author

Squashed commits!

@ivanvc
Copy link
Member

ivanvc commented Sep 30, 2024

/retest

Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @redwrasse.

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

That was hard on my eyes to review. Please next time, consider sending separate PRs for "err ==", "err !=" and "err.(*)" patterns. It would be easier to review those separately.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, ivanvc, redwrasse, serathius

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@serathius serathius merged commit c1976a6 into etcd-io:main Oct 3, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants