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

*: update go-svc #1319

Merged
merged 1 commit into from
Feb 9, 2021
Merged

*: update go-svc #1319

merged 1 commit into from
Feb 9, 2021

Conversation

jehiah
Copy link
Member

@jehiah jehiah commented Feb 2, 2021

This extracts go-svc changes out of #1305 and to update the version go-svc we import.

@jehiah jehiah self-assigned this Feb 2, 2021
go.mod Outdated Show resolved Hide resolved
@jehiah
Copy link
Member Author

jehiah commented Feb 2, 2021

RFR @mreiferson @ploxiln

@@ -9,7 +9,7 @@ import (
"syscall"

"github.com/BurntSushi/toml"
"github.com/judwhite/go-svc/svc"
svc "github.com/judwhite/go-svc"
Copy link
Contributor

Choose a reason for hiding this comment

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

@jehiah I don't think you need the import alias. I'm not using it in the example project

https://github.com/judwhite/go-svc/blob/a2deda217eb3db0e1264c3386d5e41f8c8b08207/example/main.go#L10

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm by convention i expect aliases when the package names doesn't match the directory name (i.e. svc and go-svc) but clearly there is more to goimports logic around this that I don't understand because it doesn't automatically add this alias (but leaves it if it's there).

I think either approach here is ok 🤷‍♂️

apps/nsqd/main.go Outdated Show resolved Hide resolved
apps/nsqlookupd/main.go Outdated Show resolved Hide resolved
@@ -9,7 +9,7 @@ import (
"syscall"

"github.com/BurntSushi/toml"
"github.com/judwhite/go-svc/svc"
svc "github.com/judwhite/go-svc"
"github.com/mreiferson/go-options"
Copy link
Contributor

Choose a reason for hiding this comment

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

@jehiah You know, it was the early days. Every package started with go- 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

goodness knows I have a bunch of those myself 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

@jehiah Sometimes I forget to mention what I'm trying to point out 🤦‍♂️ go-options (the line I clicked 'Comment' on) is used like options. It's no big deal, just pointing it out for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

@judwhite That's a good point (and consistency is a compelling argument)

What i didn't know when i wrote this (it took me too long to track this down) is that goimports does special case the "go-" prefix when handling import paths; I didn't know that.

https://github.com/golang/tools/blob/b894a3290fff7ed8373c3156460600f8216a6c2d/internal/imports/fix.go#L1128-L1151

@jehiah jehiah merged commit 5b9d7b4 into nsqio:master Feb 9, 2021
p.nsqd.TermSignal()
}
}()
signal.Notify(signalChan, syscall.SIGTERM)
Copy link
Member

Choose a reason for hiding this comment

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

This still feels odd to me that we have to separate out a signal to get the behavior we want. Feels like go-svc should be able to handle multiple signals, and "programs" can implement a callback act on a received signal and return something to go-svc indicating whether it should shut down?

@mreiferson mreiferson changed the title update go-svc *: update go-svc Mar 1, 2021
@mreiferson mreiferson deleted the go_svc_update_1319 branch March 1, 2021 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants