server: fix unexported-return lint issue #19052

aladesawe · 2024-12-12T22:55:52Z

PR to fix unexported-return lint issues under the server folder as mentioned in Issue 18370.

This PR:

uses existing interfaces to fix

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

k8s-ci-robot · 2024-12-12T22:56:02Z

Hi @aladesawe. 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.

ivanvc · 2024-12-13T00:06:22Z

Hi, @aladesawe. Thanks for your pull request 🙇.

One of many PRs to fix unexported-return lint issues under the server folder as mentioned in #18370.

Do you know how many issues there are in server? If there are not too many, it may be better to open a single pull request. We ideally want to keep the PR under 100 or 200 lines.

Thanks, again.

aladesawe · 2024-12-13T04:52:40Z

Hi, @aladesawe. Thanks for your pull request 🙇.

One of many PRs to fix unexported-return lint issues under the server folder as mentioned in #18370.

Do you know how many issues there are in server? If there are not too many, it may be better to open a single pull request. We ideally want to keep the PR under 100 or 200 lines.

Thanks, again.

Hello @ivanvc sure thing, I'll pull more updates into this PR. There are about 10 more files with the error under the server module.

ivanvc · 2024-12-18T21:13:32Z

/ok-to-test

codecov · 2024-12-18T22:05:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.92%. Comparing base (85931e1) to head (43c1b44).
Report is 17 commits behind head on main.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/auth/store.go	`83.79% <100.00%> (ø)`
server/etcdserver/adapters.go	`66.66% <100.00%> (ø)`
server/etcdserver/apply/apply.go	`78.02% <ø> (ø)`
server/mock/mockstorage/storage_recorder.go	`95.00% <ø> (ø)`
server/storage/mvcc/kvstore.go	`89.86% <ø> (ø)`
server/storage/mvcc/watchable_store.go	`93.89% <100.00%> (ø)`
server/storage/schema/alarm.go	`86.66% <ø> (ø)`
server/storage/schema/auth.go	`100.00% <100.00%> (ø)`
server/storage/schema/membership.go	`59.85% <ø> (ø)`
server/storage/wal/version.go	`51.53% <ø> (ø)`

... and 16 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19052      +/-   ##
==========================================
+ Coverage   68.85%   68.92%   +0.06%     
==========================================
  Files         420      420              
  Lines       35716    35723       +7     
==========================================
+ Hits        24593    24622      +29     
+ Misses       9693     9681      -12     
+ Partials     1430     1420      -10

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 85931e1...43c1b44. Read the comment docs.

ivanvc · 2024-12-19T19:24:41Z

/test pull-etcd-govulncheck

ivanvc · 2025-01-16T23:23:49Z

@aladesawe, I'm sorry it took me a while to respond to this pull request. I was out for the holidays and had a long backlog.

A heads up that I raised this question in a similar pull request: #19105 (comment)

Thanks for bearing with us :)

aladesawe · 2025-01-17T04:06:23Z

@aladesawe, I'm sorry it took me a while to respond to this pull request. I was out for the holidays and had a long backlog.

A heads up that I raised this question in a similar pull request: #19105 (comment)

Thanks for bearing with us :)

No worries Ivan, understandable :). I see the comment, and will follow the conversation there.

server/storage/wal/version.go

server/storage/mvcc/kvstore.go

ivanvc

LGTM. Thanks, @aladesawe.

server/auth/store_test.go

server/storage/mvcc/watchable_store_test.go

aladesawe · 2025-01-27T18:24:08Z

/retest

ivanvc · 2025-01-27T18:52:31Z

@aladesawe, could you please squash your commits?

ahrtr · 2025-01-31T18:38:19Z

server/auth/store.go

@@ -938,7 +938,7 @@ func (as *authStore) IsAuthEnabled() bool {
 }

 // NewAuthStore creates a new AuthStore.
-func NewAuthStore(lg *zap.Logger, be AuthBackend, tp TokenProvider, bcryptCost int) *authStore {
+func NewAuthStore(lg *zap.Logger, be AuthBackend, tp TokenProvider, bcryptCost int) AuthStore {


Nice change!

ahrtr · 2025-01-31T18:39:55Z

server/etcdserver/adapters.go

@@ -33,7 +33,7 @@ type serverVersionAdapter struct {
 	*EtcdServer
 }

-func NewServerVersionAdapter(s *EtcdServer) *serverVersionAdapter {
+func NewServerVersionAdapter(s *EtcdServer) serverversion.Server {


ahrtr · 2025-01-31T18:42:28Z

The changes, which use the existing interfaces, should can be merged right away. Could you raise a separate PR for that?

All other changes, which introduce new interfaces, really need more careful review. Please raise a PR for each new interface.

aladesawe · 2025-02-05T05:39:02Z

The changes, which use the existing interfaces, should can be merged right away. Could you raise a separate PR for that?

All other changes, which introduce new interfaces, really need more careful review. Please raise a PR for each new interface.

@ahrtr Moved all new interfaces to the own PRs

aladesawe · 2025-02-05T06:37:26Z

/retest

server/storage/wal/version.go

aladesawe · 2025-02-05T17:41:37Z

server/storage/schema/schema.go

@@ -47,10 +48,7 @@ func localBinaryVersion() semver.Version {
 	return semver.Version{Major: v.Major, Minor: v.Minor}
 }

-type WALVersion interface {


@ahrtr see here. Version below is wal.Version. Please let me know what your concerns are

OK, got it. thx for the clarification. Since the WALVersion is only included in main (release-3.5 doesn't have this interface), and we haven't release 3.6 yet. So it should be OK to rename it.

cc @serathius

@ahrtr this makes sense. I've moved the changes here

ahrtr · 2025-02-05T18:08:43Z

server/storage/schema/schema.go

@@ -47,10 +48,7 @@ func localBinaryVersion() semver.Version {
 	return semver.Version{Major: v.Major, Minor: v.Minor}
 }

-type WALVersion interface {


OK, got it. thx for the clarification. Since the WALVersion is only included in main (release-3.5 doesn't have this interface), and we haven't release 3.6 yet. So it should be OK to rename it.

cc @serathius

k8s-ci-robot · 2025-02-05T18:08:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, aladesawe, ivanvc, mmorel-35

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:

~~OWNERS~~ [ahrtr]

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

server/storage/schema/schema.go

ivanvc · 2025-02-06T07:15:36Z

Asking for a review from @ahrtr again, as @aladesawe removed the change to wal.Version.

ahrtr · 2025-02-06T09:29:07Z

server/auth/store_test.go

+	asImpl, ok := as.(*authStore)
+	require.Truef(t, ok, "addUserWithNoOption: needs an AuthStore implementation")


Can we add a validation in production code similar to below?

etcd/server/storage/schema/auth.go

Line 41 in 4abcda4

var _ auth.AuthBackend = (*authBackend)(nil)

Suggested change

asImpl, ok := as.(*authStore)

require.Truef(t, ok, "addUserWithNoOption: needs an AuthStore implementation")

ahrtr · 2025-02-06T09:29:28Z

server/storage/mvcc/watchable_store_test.go

+	sImpl, ok := s.(*watchableStore)
+	require.Truef(t, ok, "TestWatchNoEventLossOnCompact: needs a WatchableKV implementation")


ditto

Suggested change

sImpl, ok := s.(*watchableStore)

require.Truef(t, ok, "TestWatchNoEventLossOnCompact: needs a WatchableKV implementation")

This already exists

etcd/server/storage/mvcc/watchable_store.go

Line 73 in 4abcda4

var _ WatchableKV = (*watchableStore)(nil)

Confirming if also suggesting to remove the assertions?

ahrtr · 2025-02-06T09:31:43Z

server/storage/mvcc/watchable_store_test.go

+			sImpl, ok := s.(*watchableStore)
+			require.Truef(t, ok, "TestWatchBatchUnsynced: needs a WatchableKV implementation")


Exists

etcd/server/storage/mvcc/watchable_store.go

Line 73 in 4abcda4

var _ WatchableKV = (*watchableStore)(nil)

ivanvc · 2025-02-06T17:49:54Z

@aladesawe, with breaking this pull request into multiple ones. I would suggest adding a ignore comment directive (// revive:disable-next-line:unexported-return). So, technically, by merging this pull request, we can enable the linter. We address each of the special cases in different pull requests (and remove the comment directive).

aladesawe · 2025-02-06T20:01:54Z

@aladesawe, with breaking this pull request into multiple ones. I would suggest adding a ignore comment directive (// revive:disable-next-line:unexported-return). So, technically, by merging this pull request, we can enable the linter. We address each of the special cases in different pull requests (and remove the comment directive).

@ivanvc makes sense, that's good feedback. Do you anticipate we'll turn on the lint rule after this goes in, to target the server module specifically? Mind showing me how that'll be done? I had to go into the test script to localize server module while testing. I wasn't able to do include/exclude successfully via revive settings

Signed-off-by: adeyemi <[email protected]>

k8s-ci-robot · 2025-02-06T22:13:29Z

@aladesawe: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-verify	`43c1b44`	link	true	`/test pull-etcd-verify`
pull-etcd-e2e-amd64	`43c1b44`	link	true	`/test pull-etcd-e2e-amd64`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-ci-robot added the needs-ok-to-test label Dec 12, 2024

k8s-ci-robot added the size/S label Dec 12, 2024

aladesawe marked this pull request as draft December 13, 2024 04:52

k8s-ci-robot added do-not-merge/work-in-progress size/L and removed size/S labels Dec 13, 2024

aladesawe changed the title ~~auth: fix unexported-return lint issue~~ server: fix unexported-return lint issue Dec 15, 2024

aladesawe marked this pull request as ready for review December 16, 2024 22:08

k8s-ci-robot removed the do-not-merge/work-in-progress label Dec 16, 2024

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Dec 18, 2024

ivanvc mentioned this pull request Dec 5, 2024

Enable unexported-return linter rule #18370

Open

6 tasks

ivanvc reviewed Jan 22, 2025

View reviewed changes

server/storage/wal/version.go Outdated Show resolved Hide resolved

ivanvc reviewed Jan 22, 2025

View reviewed changes

server/storage/mvcc/kvstore.go Outdated Show resolved Hide resolved

ivanvc approved these changes Jan 24, 2025

View reviewed changes

mmorel-35 suggested changes Jan 25, 2025

View reviewed changes

server/auth/store_test.go Outdated Show resolved Hide resolved

server/storage/mvcc/watchable_store_test.go Outdated Show resolved Hide resolved

server/storage/mvcc/watchable_store_test.go Outdated Show resolved Hide resolved

aladesawe requested a review from mmorel-35 January 27, 2025 04:44

mmorel-35 approved these changes Jan 27, 2025

View reviewed changes

aladesawe force-pushed the server-auth-unexported-return branch from 66c71ea to e2a99cf Compare January 27, 2025 21:46

ahrtr reviewed Jan 31, 2025

View reviewed changes

aladesawe force-pushed the server-auth-unexported-return branch from e2a99cf to da82a69 Compare February 5, 2025 05:37

k8s-ci-robot added size/M and removed size/L labels Feb 5, 2025

ahrtr reviewed Feb 5, 2025

View reviewed changes

server/storage/wal/version.go Outdated Show resolved Hide resolved

aladesawe commented Feb 5, 2025

View reviewed changes

ahrtr approved these changes Feb 5, 2025

View reviewed changes

k8s-ci-robot added the approved label Feb 5, 2025

aladesawe force-pushed the server-auth-unexported-return branch from da82a69 to 977673a Compare February 5, 2025 22:31

aladesawe commented Feb 5, 2025

View reviewed changes

server/storage/schema/schema.go Outdated Show resolved Hide resolved

aladesawe force-pushed the server-auth-unexported-return branch 2 times, most recently from c3e312c to 87026bd Compare February 5, 2025 23:23

aladesawe mentioned this pull request Feb 5, 2025

Migrate WALVersion to fix unexported-return of walVersion #19350

Open

ivanvc requested a review from ahrtr February 6, 2025 07:14

ahrtr reviewed Feb 6, 2025

View reviewed changes

server: fix unexported-return using interfaces

43c1b44

Signed-off-by: adeyemi <[email protected]>

aladesawe force-pushed the server-auth-unexported-return branch from 87026bd to 43c1b44 Compare February 6, 2025 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: fix unexported-return lint issue #19052

server: fix unexported-return lint issue #19052

aladesawe commented Dec 12, 2024 •

edited

Loading

k8s-ci-robot commented Dec 12, 2024

ivanvc commented Dec 13, 2024

aladesawe commented Dec 13, 2024

ivanvc commented Dec 18, 2024

codecov bot commented Dec 18, 2024 •

edited

Loading

ivanvc commented Dec 19, 2024

ivanvc commented Jan 16, 2025

aladesawe commented Jan 17, 2025

ivanvc left a comment

aladesawe commented Jan 27, 2025

ivanvc commented Jan 27, 2025

ahrtr Jan 31, 2025

ahrtr Jan 31, 2025

ahrtr commented Jan 31, 2025

aladesawe commented Feb 5, 2025 •

edited

Loading

aladesawe commented Feb 5, 2025

aladesawe Feb 5, 2025

ahrtr Feb 5, 2025

aladesawe Feb 5, 2025

ahrtr Feb 5, 2025

k8s-ci-robot commented Feb 5, 2025

ivanvc commented Feb 6, 2025

ahrtr Feb 6, 2025

ahrtr Feb 6, 2025

aladesawe Feb 6, 2025

ahrtr Feb 6, 2025

aladesawe Feb 6, 2025

ivanvc commented Feb 6, 2025

aladesawe commented Feb 6, 2025

k8s-ci-robot commented Feb 6, 2025

		asImpl, ok := as.(*authStore)
		require.Truef(t, ok, "addUserWithNoOption: needs an AuthStore implementation")

		sImpl, ok := s.(*watchableStore)
		require.Truef(t, ok, "TestWatchNoEventLossOnCompact: needs a WatchableKV implementation")

		sImpl, ok := s.(*watchableStore)
		require.Truef(t, ok, "TestWatchBatchUnsynced: needs a WatchableKV implementation")

server: fix unexported-return lint issue #19052

Are you sure you want to change the base?

server: fix unexported-return lint issue #19052

Conversation

aladesawe commented Dec 12, 2024 • edited Loading

k8s-ci-robot commented Dec 12, 2024

ivanvc commented Dec 13, 2024

aladesawe commented Dec 13, 2024

ivanvc commented Dec 18, 2024

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

ivanvc commented Dec 19, 2024

ivanvc commented Jan 16, 2025

aladesawe commented Jan 17, 2025

ivanvc left a comment

Choose a reason for hiding this comment

aladesawe commented Jan 27, 2025

ivanvc commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Jan 31, 2025

aladesawe commented Feb 5, 2025 • edited Loading

aladesawe commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 5, 2025

ivanvc commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanvc commented Feb 6, 2025

aladesawe commented Feb 6, 2025

k8s-ci-robot commented Feb 6, 2025

aladesawe commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

aladesawe commented Feb 5, 2025 •

edited

Loading