Skip to content

Conversation

@ballista01
Copy link
Member

@ballista01 ballista01 commented Oct 23, 2025

Extracted from #136 for easier review per #136 (comment). API Changes only, no logic.

@ballista01
Copy link
Member Author

/ok-to-test

@ballista01
Copy link
Member Author

/cc @ahrtr @ivanvc The first step towards status & condition reporting instead of the old mega PR #136 :D, PTAL when you have a moment.

@ivanvc
Copy link
Member

ivanvc commented Oct 26, 2025

Link to #182

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.

This matches what's in #182. I left only a suggestion. Thanks, @ballista01

Comment on lines 159 to 161
// LeaderId is the hex-encoded ID of the current etcd cluster leader, if one exists and is known.
// +optional
LeaderId string `json:"leaderId,omitempty"`
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 rename this to LeaderID to match the Go standard?

Suggested change
// LeaderId is the hex-encoded ID of the current etcd cluster leader, if one exists and is known.
// +optional
LeaderId string `json:"leaderId,omitempty"`
// LeaderID is the hex-encoded ID of the current etcd cluster leader, if one exists and is known.
// +optional
LeaderID string `json:"leaderID,omitempty"`

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it also matches what's in etcdutils.go. Thanks for the suggestion! Applied.

@ahrtr
Copy link
Member

ahrtr commented Oct 28, 2025

Overall looks good to me.

cc @hakman @ivanvc do you have any comment?

If no, I think we can merge this PR so as not to block the other PRs which is based on this one

@ahrtr
Copy link
Member

ahrtr commented Oct 28, 2025

Thanks @ballista01 for the contribution!

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, @ballista01.

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

Thank you!

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@ahrtr ahrtr merged commit c4d7a62 into etcd-io:main Oct 28, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants