feat: bootstrap and graceful shutdown #15
Conversation
| ) | ||
|
|
||
| // ListenInterrupt listens for OS interrupt signals and calls f on receive. | ||
| // Calling ListenInterrupt blocks until a signal is received and thus must |
There was a problem hiding this comment.
[typo] I think a "be" is missing here ;)
GregoryAlbouy
left a comment
GregoryAlbouy
left a comment
There was a problem hiding this comment.
I'm sorry I had a hard time understanding this PR, starting with its purpose but also regarding some implementation choices.
I find the articulation main-run more complex than it should be, and I think this is partly due to that weird thing with shutdown.Handle 😕
| flag.Parse() | ||
| addr := ":" + *port | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| go shutdown.ListenInterrupt(cancel) |
There was a problem hiding this comment.
[question] What's the use case for this? When is the server supposed to receive an interrupt signal? Doesn't it run continuously once deployed? 🤔
| package shutdown | ||
|
|
||
| // Handle is a configurable shutdown handle that executes a | ||
| // custom shutdown procedure. | ||
| type Handle struct { | ||
| handleFunc func() error | ||
| } | ||
|
|
||
| func NewHandle(f func() error) *Handle { | ||
| return &Handle{ | ||
| handleFunc: f, | ||
| } | ||
| } | ||
|
|
||
| // Call calls the registered handlFunc on Handle. | ||
| // Invoking Call is noop if handlFunc is nil. | ||
| func (h *Handle) Call() error { | ||
| if h.handleFunc == nil { | ||
| return nil | ||
| } | ||
| return h.handleFunc() | ||
| } |
There was a problem hiding this comment.
[change required] What's this? I don't get it: It doesn't provide anything but a complex and error-prone way to only call a function declared by the consumer... Via a method Call to be called by the consumer itself!
If it has to both declare and call the function and there's nothing more involved, then what's the point?
func main() {
f, _ := run()
if f != nil { // either check for nil or assume it's not
f.Call() // no idea if myFunc was called at this point
}
}
func run() (*funccaller.Func, error) { // unusual signature
// ...
return funccaller.NewFunc(myFunc), nil
}
func myFunc() {}vs
func main() {
_ = run()
myFunc()
}
func run() error {
// ...
return nil
}
func myFunc() {}Also shutdown.Handle represents how you decided to use it in main but it doesn't quite match what it actually does (call any function via a method Call): no shutdown-related logics nor handling stuff involved here 🤔
| if err := godotenv.Load(file); err != nil { | ||
| return config, err | ||
| } |
There was a problem hiding this comment.
[change required] We can't return here: the server is deployed from the git repository where .env is ignored, so we need be able to read injected environment variables without that file.
| func defaultConfig() config { | ||
| return config{ | ||
| addr: ":" + defaultPort, | ||
| } | ||
| } |
There was a problem hiding this comment.
[suggestion] Maybe defaultConfig a bit misleading because it returns an invalid config, only the default port is set. I'd suggest to manage the port defaulting inside readConfigFile without relying on that function.
| project string | ||
| collection string |
There was a problem hiding this comment.
[question] Are collection ids / table names really part of a server config? It's not sensitive nor dependent of the environment to me, so it could be hardcoded as well. (Just wondering, not very important here)
| // If the bootstrap of the server fails, run returns a non-nil error. | ||
| // Otherwise the caller must use the shutdown.Handle to stop the server. | ||
| func run(ctx context.Context, cancel context.CancelFunc) (*shutdown.Handle, error) { | ||
| configPath := flag.String("config", defaultConfigPath, "Path to the configuration file (for example --config .env)") |
There was a problem hiding this comment.
[nitpick] It's more an environment file than a server config to me
| configPath := flag.String("config", defaultConfigPath, "Path to the configuration file (for example --config .env)") | |
| envFile := flag.String("envFile", defaultEnvFile, "Path to environment file (for example -envFile .env)") |
| } | ||
|
|
||
| rs, err := firestore.NewReportService(context.Background(), projectID, collectionID) | ||
| rs, err := firestore.NewReportService(context.Background(), config.project, config.collection) |
There was a problem hiding this comment.
[suggestion]
| rs, err := firestore.NewReportService(context.Background(), config.project, config.collection) | |
| rs, err := firestore.NewReportService(ctx, config.project, config.collection) |
|
The main thing is: In order to be able to call
Only after My solution was to:
In other words: Although I agree that Also, you raised good points regarding the env vars injections on real world deployment too. We might need to think about that. |
Description
Breaks
mainin many methods for a more granular configuration. Offers:shutdownpackage.configstruct defining the program variable configuration (à la 12 factors app), config is read from file with default being.env. Accepts one single flag:configto provide the path to the config file.Changes
Refactor
cmd/main.Update
golangci-yml:Notes
Coses #14