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

Make connectable and connection public #481

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mikroskeem
Copy link

This allows to implement new connector backends externally (e.g. HTTPS or UDS) by implementing Connectable and Connection traits.

@mikroskeem
Copy link
Author

For reference, reqwest based connector implementation

@tony-iqlusion
Copy link
Member

reqwest has several hundred dependencies which is why we don't use it, however it might make more sense to add a native HTTPS connector using a more lightweight client like ureq.

I'm not familiar with UDS... it it something you actually intend to use?

@mikroskeem
Copy link
Author

While implementing https connector in the library would cover one of the use-cases that yubihsm-connector supports, I would still love to proceed with this approach as well.

Now if I bring UDS as an example, it's not something what yubihsm-connector officially supports at first place, and is rather specific to my organization - therefore it's not worth upstreaming & maintaining here.

I believe this change could be useful for others who want to do non-standard communication and don't want to maintain a fork of this library.

@mikroskeem
Copy link
Author

Bump, is this PR still acceptable?

@tony-iqlusion
Copy link
Member

It's out-of-date with the base branch.

And... I guess, I don't understand your use case and it's more API surface to keep stable instead of being able to refactor, but I guess the counterpoint is we haven't touched it in awhile so it is defacto stable.

@mikroskeem
Copy link
Author

mikroskeem commented Jan 19, 2024

I don't know how I can explain the use-case simpler than I did in PR description... perhaps that to keep this library small and not pull in heavy dependencies like reqwest (the gist I referenced is an example that can be achieved once this PR is merged), yet library consumers being able to introduce their own transport implementations without needing to maintain their own forks.

Reason why I asked if this PR is acceptable, is to decide if I'll bother with rebasing it, or just close it.

@tony-iqlusion
Copy link
Member

I guess I can merge it if you update it

@mikroskeem mikroskeem force-pushed the feature/pub-connector-connectable branch from af09b85 to 840a43e Compare January 20, 2024 14:26
@mikroskeem
Copy link
Author

Rebase done

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.

2 participants