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.

Sign up for GitHub

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

Jump to bottom

api: restore Leader() and Peers() to avoid breaking function signatures #8395

Merged
merged 2 commits into from
Jul 29, 2020
Merged

api: restore Leader() and Peers() to avoid breaking function signatures #8395

merged 2 commits into from
Jul 29, 2020

Conversation

mikemorris
Copy link
Contributor

@mikemorris mikemorris commented Jul 28, 2020 •
edited
Loading

Refs #7818

Restores Leader() and Peers() function signatures as wrappers to avoid a breaking change, moves new implementations with QueryOptions support to LeaderWithQueryOptions(q *QueryOptions) and PeersWithQueryOptions(q *QueryOptions)

@mikemorris
Copy link
Contributor Author

mikemorris commented Jul 28, 2020 •
edited
Loading

Is there a missing integration test for the HTTP API? I feel like I should've needed to swap that from Leader to LeaderWithQueryOptions for this to work as expected... (or is the api module built on top of the HTTP API as it kinda looks from the implementation here?)

schristoff
schristoff approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@schristoff schristoff left a comment

Choose a reason for hiding this comment

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

🤙 This looks good.
Thank you!

My one nitpick is I find "got x, want y" error messages a lot more helpful - but it seems like this just copied the previous testing.

hanshasselberg
hanshasselberg reviewed Jul 29, 2020
View reviewed changes
Copy link
Member

@hanshasselberg hanshasselberg left a comment

Choose a reason for hiding this comment

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

LGTM

@mikemorris
api: add TestAPI_StatusPeersWithQueryOptions
934045e 
api: make TestAPI_Status error message more verbose
@mikemorris mikemorris merged commit 85ef7ba into master Jul 29, 2020
@mikemorris mikemorris deleted the api/status-queryoptions-wrapper branch July 29, 2020 16:09
This was referenced Jul 29, 2020
Merged
Closed
@dependabot dependabot bot mentioned this pull request Mar 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schristoff schristoff schristoff approved these changes

@hanshasselberg hanshasselberg hanshasselberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mikemorris @hanshasselberg @schristoff