search

Juniper / go-netconf

NETCONF implementation in Go.
Other
253 stars 110 forks source link

Request: make transportBasicIO public #107

Closed nights99 closed 2 years ago

nights99 commented 3 years ago

Does to make sense to make transportBasicIO public?

I needed to implement a new transport plugin, but one that I don't think makes sense to add to this package; extending transportBasicIO meant it was much easier to implement - fewer functions required to be implemented.

Does it make sense to make it public, or have I missed another way? The change is very easy, and I'm happy to submit as a PR, but thought it worth checking first.

Thanks, Jon

nemith commented 2 years ago

transportBasicIO shouldn't ever have been a thing. This was my first go project and i was looking for subclassing which isn't very Go idomatic. I ok with making it public but taking a different approach with a v2 rewrite: https://github.com/nemith/go-netconf/blob/v2/transport/transport.go

What transport are you adding? TLS? or something else? Curious as I want to make sure the rewrite is pluggable.

nights99 commented 2 years ago

transportBasicIO shouldn't ever have been a thing. This was my first go project and i was looking for subclassing which isn't very Go idomatic. I ok with making it public but taking a different approach with a v2 rewrite: https://github.com/nemith/go-netconf/blob/v2/transport/transport.go

What transport are you adding? TLS? or something else? Curious as I want to make sure the rewrite is pluggable.

Possibly a bit odd, but I'm compiling the code to WebASM, which works other than not having access to normal sockets - so I've added a very simple WebSocket transport, and then have a simple proxy that copies the Websocket across to a ssh session.

nemith commented 2 years ago

Yeah totally makes sense. I am making sure that transports are pluggable in a rewrite. Open to feedback on the current model. It should fit websockets quite well

nights99 commented 2 years ago

Submitted a PR as its still useful for me to have this in the existing repo.

Separately I've tried the v2 re-write and its definitely looking good :)