Disable revive on NewMembershipBackend to fix unexported-return #19343

aladesawe · 2025-02-05T05:21:37Z

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

k8s-ci-robot · 2025-02-05T05:21:48Z

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.

aladesawe · 2025-02-05T05:21:54Z

Pulled from PR

codecov · 2025-02-05T05:39:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.83%. Comparing base (eb7607b) to head (f312736).
Report is 12 commits behind head on main.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/storage/schema/membership.go	`59.85% <ø> (ø)`

... and 26 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19343      +/-   ##
==========================================
- Coverage   68.89%   68.83%   -0.06%     
==========================================
  Files         420      420              
  Lines       35753    35753              
==========================================
- Hits        24632    24611      -21     
- Misses       9698     9719      +21     
  Partials     1423     1423

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 eb7607b...f312736. Read the comment docs.

ahrtr · 2025-02-06T14:28:34Z

/ok-to-test

ahrtr · 2025-02-06T14:43:26Z

server/storage/schema/membership.go

+type MembershipBackend interface {
+	MustSaveMemberToBackend(m *membership.Member)
+	TrimClusterFromBackend() error
+	MustDeleteMemberFromBackend(id types.ID)
+	MustReadMembersFromBackend() (map[types.ID]*membership.Member, map[types.ID]bool)
+	TrimMembershipFromBackend() error
+	MustSaveClusterVersionToBackend(ver *semver.Version)
+	MustSaveDowngradeToBackend(downgrade *version.DowngradeInfo)
+	MustCreateBackendBuckets()
+	ClusterVersionFromBackend() *semver.Version
+	DowngradeInfoFromBackend() *version.DowngradeInfo
+}


It's duplicated to

etcd/server/etcdserver/api/membership/store.go

Lines 27 to 32 in f1d09c0

type MembershipBackend interface {

ClusterVersionBackend

MemberBackend

DowngradeInfoBackend

MustCreateBackendBuckets()

}

Again, the simplest solution is to move it into storage/schema

@ahrtr do we want to import into schema versus moving it to schema? There's been no exported interface definition in schema

@ahrtr Moving can't be done without a cyclic import; please refer to commit. tldr MembershipBackend is still referenced within the membership package

@ahrtr Moving can't be done without a cyclic import; please refer to commit. tldr MembershipBackend is still referenced within the membership package

thx for pointing this out. Unfortunately, this existing design/implementation has some serious flaws. The server/etcdserver/api/* and server/storage/* are referencing (depending on) each other.

Ideally, they shouldn't depend on each other, instead both of them should only depend on the same set of interfaces (abstract).

The less optimised solution is to apply only one way reference (dependence)

Let me see what I can do.

Unfortunately it needs big refactor to resolve this. So I suggest that we only add // revive:disable-next-line:unexported-return to ignore it for now. In future (i.e 3.8 or 4.0), when we get rid of v2snapshot, we can revisit it.

ahrtr · 2025-02-18T10:17:14Z

server/storage/schema/membership.go

@@ -37,7 +37,7 @@ type membershipBackend struct {
 	be backend.Backend
 }

-func NewMembershipBackend(lg *zap.Logger, be backend.Backend) *membershipBackend {
+func NewMembershipBackend(lg *zap.Logger, be backend.Backend) membership.MembershipBackend {


Suggested change

func NewMembershipBackend(lg *zap.Logger, be backend.Backend) membership.MembershipBackend {

// Refer to https://github.com/etcd-io/etcd/pull/19343#discussion_r1958056718

// revive:disable-next-line:unexported-return

func NewMembershipBackend(lg *zap.Logger, be backend.Backend) *membershipBackend {

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

ahrtr

LGTM

Thanks

k8s-ci-robot · 2025-02-20T18:48:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

k8s-ci-robot · 2025-02-20T18:53:46Z

@aladesawe: The following test 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-coverage-report	`f312736`	link	false	`/test pull-etcd-coverage-report`

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.

aladesawe · 2025-02-20T20:01:16Z

/retest

k8s-ci-robot added the needs-ok-to-test label Feb 5, 2025

k8s-ci-robot added the size/S label Feb 5, 2025

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Feb 6, 2025

ahrtr reviewed Feb 6, 2025

View reviewed changes

aladesawe changed the title ~~Create an MembershipBackend interface to fix unexported-return~~ Create a MembershipBackend interface to fix unexported-return Feb 8, 2025

aladesawe force-pushed the server-membership-unexported-return branch from 8755557 to e3086e3 Compare February 17, 2025 05:49

k8s-ci-robot added size/M and removed size/S labels Feb 17, 2025

aladesawe force-pushed the server-membership-unexported-return branch from e3086e3 to 8dc493e Compare February 17, 2025 05:52

aladesawe marked this pull request as draft February 17, 2025 05:58

k8s-ci-robot added the do-not-merge/work-in-progress label Feb 17, 2025

aladesawe force-pushed the server-membership-unexported-return branch from 8dc493e to b45c3e7 Compare February 17, 2025 06:48

k8s-ci-robot added size/XS and removed size/M labels Feb 17, 2025

aladesawe marked this pull request as ready for review February 17, 2025 06:56

k8s-ci-robot removed the do-not-merge/work-in-progress label Feb 17, 2025

ahrtr reviewed Feb 18, 2025

View reviewed changes

aladesawe changed the title ~~Create a MembershipBackend interface to fix unexported-return~~ Disable revive on NewMembershipBackend to fix unexported-return Feb 19, 2025

aladesawe marked this pull request as draft February 19, 2025 05:58

k8s-ci-robot added the do-not-merge/work-in-progress label Feb 19, 2025

Disable revive for NewMembershipBackend to fix unexported-return

f312736

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

aladesawe force-pushed the server-membership-unexported-return branch from b45c3e7 to f312736 Compare February 20, 2025 18:31

aladesawe marked this pull request as ready for review February 20, 2025 18:31

k8s-ci-robot removed the do-not-merge/work-in-progress label Feb 20, 2025

ahrtr approved these changes Feb 20, 2025

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable revive on NewMembershipBackend to fix unexported-return #19343

Disable revive on NewMembershipBackend to fix unexported-return #19343

aladesawe commented Feb 5, 2025

k8s-ci-robot commented Feb 5, 2025

aladesawe commented Feb 5, 2025

codecov bot commented Feb 5, 2025 •

edited

Loading

ahrtr commented Feb 6, 2025

ahrtr Feb 6, 2025

aladesawe Feb 8, 2025

ahrtr Feb 16, 2025

aladesawe Feb 17, 2025 •

edited

Loading

ahrtr Feb 17, 2025

ahrtr Feb 17, 2025

ahrtr Feb 18, 2025 •

edited

Loading

ahrtr left a comment

k8s-ci-robot commented Feb 20, 2025

k8s-ci-robot commented Feb 20, 2025

aladesawe commented Feb 20, 2025

	type MembershipBackend interface {
	ClusterVersionBackend
	MemberBackend
	DowngradeInfoBackend
	MustCreateBackendBuckets()
	}

Disable revive on NewMembershipBackend to fix unexported-return #19343

Are you sure you want to change the base?

Disable revive on NewMembershipBackend to fix unexported-return #19343

Conversation

aladesawe commented Feb 5, 2025

k8s-ci-robot commented Feb 5, 2025

aladesawe commented Feb 5, 2025

codecov bot commented Feb 5, 2025 • edited Loading

Codecov Report

ahrtr commented Feb 6, 2025

ahrtr Feb 6, 2025

Choose a reason for hiding this comment

aladesawe Feb 8, 2025

Choose a reason for hiding this comment

ahrtr Feb 16, 2025

Choose a reason for hiding this comment

aladesawe Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

ahrtr Feb 17, 2025

Choose a reason for hiding this comment

ahrtr Feb 17, 2025

Choose a reason for hiding this comment

ahrtr Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 20, 2025

k8s-ci-robot commented Feb 20, 2025

aladesawe commented Feb 20, 2025

codecov bot commented Feb 5, 2025 •

edited

Loading

aladesawe Feb 17, 2025 •

edited

Loading

ahrtr Feb 18, 2025 •

edited

Loading