Retrieve error handling #453
Conversation
liztio
force-pushed
the
retrieve-error-handling
branch
4 times, most recently
from
673bbcb to
f82915b
Compare
timothysc
added
the
lifecycle/triage
timothysc
added
lifecycle/consideration
pkg/client/retrieve.go
Outdated
| }, scheme.ParameterCodec) | ||
| executor, err := remotecommand.NewSPDYExecutor(c.RestConfig, "POST", req.URL()) | ||
| if err != nil { | ||
| return nil, err | ||
| ec <- err | ||
| return nil, ec | ||
| } | ||
| reader, writer := io.Pipe() | ||
| go func(writer *io.PipeWriter) { |
There was a problem hiding this comment.
we should be consistent with passing vars into this func. we should either pass the ec chan error into this func or take out writer. I prefer passing it in to make it explicit, but it doesn't really matter since this isn't in a for loop.
| @@ -34,10 +32,19 @@ import ( | |||
| "k8s.io/client-go/tools/remotecommand" | |||
| ) | |||
|
|
|||
| func (c *SonobuoyClient) RetrieveResults(cfg *RetrieveConfig) (io.Reader, error) { | |||
| var tarCommand = []string{ | |||
There was a problem hiding this comment.
why do we need to wrap this call with bash -c?
There was a problem hiding this comment.
because the sh execution doesn't handle the * globbing
There was a problem hiding this comment.
were you running into issues with the existing code?
There was a problem hiding this comment.
Yes. If you ran the command before sonobuoy had finished running, it would tar up and retrieve a temporary directory that didn't have anything useful in it. Users had to detect if the untarred file ended in .tar.gz to determine if it was successful.
| fmt.Sprintf("tar cf - %s/*.tar.gz", config.MasterResultsPath), | ||
| } | ||
|
|
||
| func (c *SonobuoyClient) RetrieveResults(cfg *RetrieveConfig) (io.Reader, <-chan error) { |
There was a problem hiding this comment.
From a client perspective, the channel makes the signature appear non-blocking, when we indeed do want the function to be blocking with a possible list of errors.
There was a problem hiding this comment.
This can't really be blocking if we want to keep the io.Reader contract. In the correct case, the error code isn't available until after execution is completed. In the error case, it could be an arbitrary amount of time (10 or more seconds) until we get the report of the status code.
There was a problem hiding this comment.
I don't mind blocking for (X) with a ctx arg provided, that is standard go. Otherwise, we need to revisit the whole interface.
Signed-off-by: liz <[email protected]>
liztio
force-pushed
the
retrieve-error-handling
branch
from
f82915b to
7ed0709
Compare
sonobuoy retrieve now has better, more user-friendly error handling closes vmware-tanzu#407 Signed-off-by: liz <[email protected]>
liztio
force-pushed
the
retrieve-error-handling
branch
from
7ed0709 to
61b1e29
Compare
What this PR does / why we need it:
Right now
sonobuoy retrieveprovides very bad error handling. This improves it:Which issue(s) this PR fixes
Special notes for your reviewer:
Release note: