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

SIGHUP is only useful when config was loaded from a file #1030

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

forfuncsake
Copy link
Contributor

Have (*config.C).CatchHUP() return early when there is no file path available from which to reload.
This will allow wrapping service to manage their own signal trapping (which is particularly important if they've used config from a string.

Have (*config.C).CatchHUP() return early when there is no file
path available from which to reload.
This will allow wrapping service to manage their own signal
trapping (which is particularly important if they've used
config from a string.
@wadey wadey added this to the v1.8.0 milestone Dec 4, 2023
@nbrownus
Copy link
Collaborator

nbrownus commented Dec 4, 2023

We had discussed removing C.CatchHUP() from the config package entirely and having the cmd package handle the call to C.ReloadConfig(). I think this is the more correct approach for anyone using an alternate config loading path (likely as a library) and presumably would enjoy having more control of the signal handling.

Happy to merge but would also like to come back and do the other work as after 1.8.

Copy link
Member

@wadey wadey left a comment

Choose a reason for hiding this comment

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

Approved, but lets come back and make this interface better.

@wadey wadey merged commit 0209402 into master Dec 6, 2023
7 checks passed
@wadey wadey deleted the config-reload-hook branch December 6, 2023 15:13
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.

3 participants