-
Notifications
You must be signed in to change notification settings - Fork 176
Add Log Tail Wrapper #179
Add Log Tail Wrapper #179
Conversation
|
Ah okay, I'll go back to the drawing board and refactor 👌 |
Ok I have made the change and tested locally and it works. Really unsure why the ci test are failing, however I don't think its related to this code, as the same problem is effecting your ci build for #180 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with two nits.
logger.go
Outdated
return Logger{}, nil | ||
} | ||
if resp.Error != nil { | ||
resp.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call resp.Output.Close()
resp.Close()` drains the stream (needed for other requests where closing early will abort the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point.
logger_test.go
Outdated
"testing" | ||
) | ||
|
||
func Test_Logger(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: TestLogger
logger.go
Outdated
|
||
resp, err := s.Request("log/tail").Send(ctx) | ||
if err != nil { | ||
return Logger{}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the bug, it should return err
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes good catch!
Ok I believe I've made the fixes. If wanted I can also port this logging stuff over to |
That's mostly for exposing a stable, "core" API so we'll have to think through this a bit. IMO, we should consider simply exposing raw requests and leaving it at that (but I'll let @magik6k make the hard decisions here :)). |
Logging is rather go-ipfs specific, and afaik there were some plans to rewrite go-log, so the interface might not be too stable. I want to rewrite most of go-ipfs-api on top of CoreAPI (with things like logging using raw requests if on top of go-ipfs-http-client, erroring if not). I should be able to PR something this week. Options here are:
|
I needed this functionality to be able to tail ipfs logs for another project and figured it would probably be useful to others.
I tried using the built in request builder, however it appears that it just continually waits for the call to finish, and given that the
log/tail
call streams responses, so it just hangs.