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

Add runtime flag to allow disabling config hot-reload (#5079) #5112

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kralicky
Copy link
Contributor

@kralicky kralicky commented May 13, 2024

Summary

This adds the runtime flag config_hot_reload (default true) that allows enabling or disabling the hot-reload mechanism for the config file and any other files referenced within it.

Related issues

#5079

User Explanation

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@kralicky kralicky added the enhancement New feature or request label May 13, 2024
@kralicky
Copy link
Contributor Author

kralicky commented May 13, 2024

Also implemented an alternative solution to this issue here: d75e1ca

@kralicky kralicky marked this pull request as ready for review May 14, 2024 18:59
@kralicky kralicky requested a review from a team as a code owner May 14, 2024 18:59
@coveralls
Copy link

coveralls commented May 14, 2024

Coverage Status

coverage: 56.662% (+0.2%) from 56.512%
when pulling 2a1db3b on kralicky/disable-hot-reload
into 568e99f on main.

Copy link
Contributor

@kenjenkins kenjenkins left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

if cfg.Options.IsRuntimeFlagSet(RuntimeFlagConfigHotReload) {
src.watcher.Watch(ctx, []string{configFile})
} else {
log.Warn(ctx).Msg("hot reload disabled")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we say not to log at warning level (see https://www.pomerium.com/docs/community/contributing#logging) — although I'm sure we're not 100% consistent about this. Still, maybe log at Info() here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants