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

Add SSH agent client with an example #44

Merged
merged 3 commits into from
Apr 22, 2024
Merged

Add SSH agent client with an example #44

merged 3 commits into from
Apr 22, 2024

Conversation

wiktor-k
Copy link
Owner

@wiktor-k wiktor-k commented Apr 17, 2024

I'm not happy with the API (especially the Client requiring type for the ListeningSocket) but overall this works so I'm sharing for any potential feedback :)

@wiktor-k wiktor-k force-pushed the client branch 5 times, most recently from 9ea1135 to 62244c9 Compare April 17, 2024 09:48
@wiktor-k wiktor-k marked this pull request as ready for review April 17, 2024 13:12
@jcspencer
Copy link
Collaborator

Looks great! I've added a few comments around the error handling (it may be good to expose a clearer error to the consumer, rather than returning UnexpectedResponse), but otherwise this looks great! The example is super clear as well, nice work!

@wiktor-k
Copy link
Owner Author

Great comments about the errors! As I mentioned in the other comment I think this is a bigger problem and we should be migrating away from the Box into maybe a more specific error type... I'll check it out in another PR but I consider it mostly orthogonal to this PR (as it applies to all others too :( ).

@wiktor-k wiktor-k force-pushed the client branch 5 times, most recently from d3a7d18 to 651bbcc Compare April 19, 2024 10:45
@jcspencer
Copy link
Collaborator

Looks great to me! The API here (mirroring the agent-side Session) is very clean!

Fixes: #32
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@wiktor-k
Copy link
Owner Author

Looks great to me! The API here (mirroring the agent-side Session) is very clean!

Haha, thanks! 😊 It occurred to me that thanks to the trait being the same one can build and agent that simply forwards requests to another agent with relative ease :)

I'm going to merge it now to avoid API drift even though I had a couple of tweaks in mind.

@wiktor-k wiktor-k merged commit 7dc92d7 into main Apr 22, 2024
18 checks passed
@wiktor-k wiktor-k deleted the client branch April 22, 2024 07:17
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