Skip to content
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

Ignoe derp.yaml, don't panic in Serve() #248

Merged
merged 8 commits into from
Dec 7, 2021
Merged

Conversation

negbie
Copy link
Contributor

@negbie negbie commented Dec 1, 2021

If there is no specifc reason to panic I think it's better to just return the error.

@negbie
Copy link
Contributor Author

negbie commented Dec 2, 2021

@kradalby Do you know why the integration-test for the second pr fails? It adds only more logging.

@kradalby
Copy link
Collaborator

kradalby commented Dec 2, 2021

They are a bit flaky, I think they are a bit more resource hungry than actions wants to so they might get in an awkward state.

Reruns usually helps...

@negbie
Copy link
Contributor Author

negbie commented Dec 2, 2021

Makes sense. Thanks for the info!

app.go Outdated
@@ -418,12 +418,12 @@ func (h *Headscale) Serve() error {

err = h.ensureUnixSocketIsAbsent()
if err != nil {
panic(err)
return err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is sensible,

But could you wrap these errors to give the user more context, when we are first at improving :)

So:

Suggested change
return err
return fmt.Errorf("description: %w", err)

app.go Outdated Show resolved Hide resolved
app.go Outdated Show resolved Hide resolved
negbie and others added 2 commits December 7, 2021 11:44
Co-authored-by: Kristoffer Dalby <kradalby@kradalby.no>
Co-authored-by: Kristoffer Dalby <kradalby@kradalby.no>
@kradalby kradalby merged commit 3fb3db6 into juanfont:main Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants