varlink / go

Golang implementation of the Varlink protocol
https://godoc.org/github.com/varlink/go/varlink
Apache License 2.0
64 stars 57 forks source link

Add upgrade method #19

Closed johanbrandhorst closed 4 years ago

johanbrandhorst commented 4 years ago

varlink: add Upgrade method to connection

Upgrade can be used to upgrade a connection for sending data. The connection cannot be reused after upgrading.

cmd/varlink-go-interface-generator: Generate Upgrade

Upgrade can be used to get access to the raw underlying connection with the varlink server.

Fixes #18

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 351


Totals Coverage Status
Change from base Build 350: 1.4%
Covered Lines: 653
Relevant Lines: 775

💛 - Coveralls
coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 358


Totals Coverage Status
Change from base Build 355: 1.4%
Covered Lines: 653
Relevant Lines: 775

💛 - Coveralls
johanbrandhorst commented 4 years ago

Thanks for this PR @johanbrandhorst ! this solves my use case just fine. I think this can close the issue #16

Glad to hear it. I ended up not needing this for my own use case in the end, but it seems reasonable to add anyway. Could you share the code you ended up writing for this?

ffromani commented 4 years ago

Thanks for this PR @johanbrandhorst ! this solves my use case just fine. I think this can close the issue #16

Glad to hear it. I ended up not needing this for my own use case in the end, but it seems reasonable to add anyway. Could you share the code you ended up writing for this?

Sure, its' all here: https://github.com/fromanirh/pack8s

At this moment the published code includea a fork of varlink/go with an horrible, horrible hack I added to keep going. On my box I applied this PR on my fork (https://github.com/fromanirh/varlink-go), which I'm going to upload later today.

To be specific, the code I mentioned I'm using to consume the podman API is here: https://github.com/fromanirh/pack8s/tree/master/pkg/varlinkapi

haraldh commented 4 years ago

Hmm, the server needs Write and Read after the connection is upgraded.

haraldh commented 4 years ago

I needed

diff --git a/varlink/call.go b/varlink/call.go
index 50dba0f..db57c06 100644
--- a/varlink/call.go
+++ b/varlink/call.go
@@ -16,7 +16,7 @@ type WriterContext interface {
 // client can be terminated by returning an error from the call instead
 // of sending a reply or error reply.
 type Call struct {
-       Conn      WriterContext
+       Conn      ReadWriterContext
        Request   *[]byte
        In        *serviceCall
        Continues bool
diff --git a/varlink/service.go b/varlink/service.go
index af8aa25..9d35fb5 100644
--- a/varlink/service.go
+++ b/varlink/service.go
@@ -76,7 +76,7 @@ func (s *Service) getInterfaceDescription(ctx context.Context, c Call, name stri
        return c.replyGetInterfaceDescription(ctx, description)
 }

-func (s *Service) HandleMessage(ctx context.Context, conn WriterContext, request []byte) error {
+func (s *Service) HandleMessage(ctx context.Context, conn ReadWriterContext, request []byte) error {
        var in serviceCall

        err := json.Unmarshal(request, &in)

which probably makes a separate WriterContext obsolete

johanbrandhorst commented 4 years ago

I have merged WriterContext into ReadWriterContext. @haraldh is this better?

haraldh commented 4 years ago

could you also rename ReadBytes to something like ReadUntilByte

johanbrandhorst commented 4 years ago

ReadBytes is consistent with the standard library: https://golang.org/pkg/bufio/#Reader.ReadBytes. I would rather not change it. Are you sure?

haraldh commented 4 years ago

Oh.. sorry.. all ok, then

johanbrandhorst commented 4 years ago

Thanks!

ffromani commented 4 years ago

Thanks for merging this PR!

haraldh commented 4 years ago

tagged as release v0.3.0

haraldh commented 4 years ago

updated https://github.com/haraldh/govarlinksendfileexample with contexts