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

feat(client): Export DoSync/DoAsync and RequestInfo. #289

Closed
wants to merge 2 commits into from

Conversation

anpep
Copy link
Collaborator

@anpep anpep commented Aug 25, 2023

This patch exports client.Do{Sync,Async} methods and moves the arguments to a RequestInfo struct. The idea is that applications extending Pebble can call these helper methods in order to perform HTTP requests to the daemon through an extended client (see #285).

@anpep anpep self-assigned this Aug 25, 2023
@anpep anpep marked this pull request as ready for review August 25, 2023 10:16
@flotter
Copy link
Contributor

flotter commented Aug 25, 2023

This PR is required to truly make the Client API changes in #285 usable from a client extension implementation. This is currently in draft form, mainly to capture some proposals on how to best do this.

@flotter flotter removed the request for review from paul-rodriguez August 25, 2023 10:18
client/client.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@paul-rodriguez paul-rodriguez left a comment

Choose a reason for hiding this comment

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

Naming suggestions

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Approved, but some minor comments.

client/client.go Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
client/changes.go Outdated Show resolved Hide resolved
client/client.go Show resolved Hide resolved
client/client.go Show resolved Hide resolved
@benhoyt
Copy link
Contributor

benhoyt commented Aug 28, 2023

ResultInfo doesn't have the Result. It is a placeholder. I would prefer Request.

@hpidcock That's fair. I'd be fine with Request for this -- it does make more sense than "request info".

@anpep
Copy link
Collaborator Author

anpep commented Sep 3, 2023

Closing this because it's basically exposing an implementation detail from the Pebble client that might not be suitable for other clients. Will iterate on a new proposal soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants