network-tunnel: exit without logging an error on EOF before READY #2085
Conversation
mdibaiee
requested review from
jgraettinger,
williamhbaker,
willdonnelly and
Alex-Bair
| // EOF means the underlying process exit without writing READY, this is most likely | ||
| // an error that we would like to surface to the user without logging anything further | ||
| // ourselves | ||
| if err == io.EOF { |
There was a problem hiding this comment.
We're doing something similar in filesource captures where the parser exits with an error and we don't want to clobber that with a less useful connector error, using a specific error type that is handled in the boilerplate, see here for where that happens. I think it would be good to do that here as well.
There was a problem hiding this comment.
@williamhbaker that required us to capture the error line from the underlying process, so I added a bit of extra machinery to capture the last error from stderr of network-tunnel and use that to bubble up a transparent error. I kept the stderr piping just in case for now. The result looks like this now:
There was a problem hiding this comment.
👍 I think it would also be fine to return the EOF error as cerrors.NewTransparentError(err) and not worry about capturing the last log line as an error. Since it is already being logged to stderr we shouldn't need to also retain its text in the Go program I don't think.
The actual err value that is wrapped is not currently actually used (maybe it shouldn't be part of TransparentError at all), in the case of the filesource capture it is a generic "Subprocess exited with non-zero code" and we are trusting the subprocess to log the actual useful error to stderr on its own.
There was a problem hiding this comment.
ah I see, I was testing using go test -v but there the TransparentError kept coming up, but I just tested using our integration tests and there is no need for us to capture the last error, so I removed those parts and I'm now simply returning the TransparentError(EOF)
mdibaiee
force-pushed
the
mahdi/network-tunnel-error-surfacing
branch
from
864cf2e to
a3e6a1f
Compare
Description:
Sister pull-request of estuary/network-tunnel#9 to surface the underlying SSH client error when network-tunnel fails to establish a SSH tunnel
Example output:
Workflow steps:
(How does one use this feature, and how has it changed)
Documentation links affected:
(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)
Notes for reviewers:
(anything that might help someone review this PR)
This change is