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: fix unexported-return lint issue #19052

Merged
merged 1 commit into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion server/auth/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import (
"go.etcd.io/etcd/api/v3/v3rpc/rpctypes"
)

var _ AuthStore = (*authStore)(nil)

var (
rootPerm = authpb.Permission{PermType: authpb.READWRITE, Key: []byte{}, RangeEnd: []byte{0}}

Expand Down Expand Up @@ -938,7 +940,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 {
Copy link
Member

Choose a reason for hiding this comment

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

Nice change!

if lg == nil {
lg = zap.NewNop()
}
Expand Down
8 changes: 5 additions & 3 deletions server/auth/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,14 @@ func setupAuthStore(t *testing.T) (store *authStore, teardownfunc func(t *testin

// The UserAdd function cannot generate old etcd version user data (user's option is nil)
// add special users through the underlying interface
addUserWithNoOption(as)
asImpl, ok := as.(*authStore)
require.Truef(t, ok, "addUserWithNoOption: needs an AuthStore implementation")
Comment on lines +116 to +117
Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

addUserWithNoOption(asImpl)

tearDown := func(_ *testing.T) {
as.Close()
}
return as, tearDown
return asImpl, tearDown
}

func addUserWithNoOption(as *authStore) {
Expand All @@ -133,7 +135,7 @@ func addUserWithNoOption(as *authStore) {
as.refreshRangePermCache(tx)
}

func enableAuthAndCreateRoot(as *authStore) error {
func enableAuthAndCreateRoot(as AuthStore) error {
_, err := as.UserAdd(&pb.AuthUserAddRequest{Name: "root", HashedPassword: encodePassword("root"), Options: &authpb.UserAddOptions{NoPassword: false}})
if err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion server/etcdserver/adapters.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ type serverVersionAdapter struct {
*EtcdServer
}

func NewServerVersionAdapter(s *EtcdServer) *serverVersionAdapter {
func NewServerVersionAdapter(s *EtcdServer) serverversion.Server {
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

return &serverVersionAdapter{
EtcdServer: s,
}
Expand Down
1 change: 1 addition & 0 deletions server/storage/mvcc/kvstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ type store struct {

// NewStore returns a new store. It is useful to create a store inside
// mvcc pkg. It should only be used for testing externally.
// revive:disable-next-line:unexported-return this is used internally in the mvcc pkg
func NewStore(lg *zap.Logger, b backend.Backend, le lease.Lessor, cfg StoreConfig) *store {
if lg == nil {
lg = zap.NewNop()
Expand Down
2 changes: 1 addition & 1 deletion server/storage/mvcc/watchable_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ var _ WatchableKV = (*watchableStore)(nil)
// cancel operations.
type cancelFunc func()

func New(lg *zap.Logger, b backend.Backend, le lease.Lessor, cfg StoreConfig) *watchableStore {
func New(lg *zap.Logger, b backend.Backend, le lease.Lessor, cfg StoreConfig) WatchableKV {
s := newWatchableStore(lg, b, le, cfg)
s.wg.Add(2)
go s.syncWatchersLoop()
Expand Down
21 changes: 13 additions & 8 deletions server/storage/mvcc/watchable_store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestWatch(t *testing.T) {
defer w.Close()

w.Watch(0, testKey, nil, 0)
if !s.synced.contains(string(testKey)) {
if !s.(*watchableStore).synced.contains(string(testKey)) {
// the key must have had an entry in synced
t.Errorf("existence = false, want true")
}
Expand All @@ -67,7 +67,7 @@ func TestNewWatcherCancel(t *testing.T) {
t.Error(err)
}

if s.synced.contains(string(testKey)) {
if s.(*watchableStore).synced.contains(string(testKey)) {
// the key shoud have been deleted
t.Errorf("existence = true, want false")
}
Expand Down Expand Up @@ -340,7 +340,9 @@ func TestWatchNoEventLossOnCompact(t *testing.T) {
require.NoError(t, err)
}
// fill up w.Chan() with 1 buf via 2 compacted watch response
s.syncWatchers([]mvccpb.Event{})
sImpl, ok := s.(*watchableStore)
require.Truef(t, ok, "TestWatchNoEventLossOnCompact: needs a WatchableKV implementation")
Comment on lines +343 to +344
Copy link
Member

Choose a reason for hiding this comment

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

ditto

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This already exists

var _ WatchableKV = (*watchableStore)(nil)

Confirming if also suggesting to remove the assertions?

sImpl.syncWatchers([]mvccpb.Event{})

for len(watchers) > 0 {
resp := <-w.Chan()
Expand All @@ -355,7 +357,7 @@ func TestWatchNoEventLossOnCompact(t *testing.T) {
require.Equalf(t, nextRev, ev.Kv.ModRevision, "got event revision %d but want %d for watcher with watch ID %d", ev.Kv.ModRevision, nextRev, resp.WatchID)
nextRev++
}
if nextRev == s.rev()+1 {
if nextRev == sImpl.rev()+1 {
delete(watchers, resp.WatchID)
}
}
Expand Down Expand Up @@ -566,10 +568,13 @@ func TestWatchBatchUnsynced(t *testing.T) {
}
assert.Equal(t, tc.expectRevisionBatches, revisionBatches)

s.store.revMu.Lock()
defer s.store.revMu.Unlock()
assert.Equal(t, 1, s.synced.size())
assert.Equal(t, 0, s.unsynced.size())
sImpl, ok := s.(*watchableStore)
require.Truef(t, ok, "TestWatchBatchUnsynced: needs a WatchableKV implementation")
Comment on lines +571 to +572
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exists

var _ WatchableKV = (*watchableStore)(nil)


sImpl.store.revMu.Lock()
defer sImpl.store.revMu.Unlock()
assert.Equal(t, 1, sImpl.synced.size())
assert.Equal(t, 0, sImpl.unsynced.size())
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion server/storage/schema/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ type authBackend struct {

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

func NewAuthBackend(lg *zap.Logger, be backend.Backend) *authBackend {
func NewAuthBackend(lg *zap.Logger, be backend.Backend) auth.AuthBackend {
return &authBackend{
be: be,
lg: lg,
Expand Down