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

Allow embedding dynamic config in static config #6573

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dnr
Copy link
Member

@dnr dnr commented Sep 27, 2024

What changed?

PoC for #6561:

  • If static config has a key dynamicConfig, use the contents of that as dynamic config.
  • dynamicConfigClient.pollInterval is allowed to be zero, meaning "do not reload".

This only works with configs loaded from yaml files, not generated by code and passed with WithConfig. Callers can use WithDynamicConfigClient with a StaticClient or MemoryClient.

Why?

Simplify server configuration for dev/test servers.

How did you test it?

unit tests, manual test of dev server (not cli)

@@ -103,6 +105,21 @@ func Load(env string, configDir string, zone string, config interface{}) error {
}
}

// Inject self-dynamic-config-client if dynamicConfig section present.
if config.DynamicConfig != nil {
if config.DynamicConfigClient == nil {
Copy link
Member

Choose a reason for hiding this comment

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

When is it okay for this not to be nil if dynamic config is specified here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it allowed to leave out dynamicConfigClient.. the only thing in there is pollInterval, if that's zero it'll just not do the refresh loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh your question was the opposite. Then the answer is: there should be a way to specify nonzero pollInterval if you want to get updates. It's easiest to just use the existing setting.

@bergundy
Copy link
Member

Thanks for doing this!

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