-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix error logging in tailscale #7776
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #7776 +/- ##
===========================================
+ Coverage 19.85% 47.05% +27.20%
===========================================
Files 83 143 +60
Lines 7686 13211 +5525
===========================================
+ Hits 1526 6217 +4691
+ Misses 5930 5899 -31
- Partials 230 1095 +865
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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 on error message
pkg/vpn/vpn.go
Outdated
if err != nil { | ||
return err | ||
return errors.New("Error while starting the VPN: " + output) |
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.
- Errors should not be capitalized
- Maybe wrap the exec error instead of creating a new one?
return errors.New("Error while starting the VPN: " + output) | |
return errors.Wrap(err, "tailscale up failed: " + output) |
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.
makes sense
6057e49
to
632fb37
Compare
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 once ci is happy
Signed-off-by: Manuel Buil <[email protected]>
632fb37
to
43611bb
Compare
Proposed Changes
Return the output of the command instead of the error. Error will say nothing useful, just "exit 1/2", whereas the output (which is cmd.Stderr) will provide more useful information
Types of Changes
Bugfix
Verification
Start VPN with a wrong joinKey. Now you should see a useful error saying:
Before, you'd only see
exit 1
Testing
Linked Issues
#7771
User-Facing Change
Further Comments