Skip to content

Add Config to FlagConfig struct #332

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

peppi-lotta
Copy link

This refactoring allows the Serve function to be called without requiring a YAML configuration file. As a result, the function becomes easier to use from other projects, since configuration is no longer tied to an external YAML file.

The change was proposed in PR #7255 to the CoreDNS project. To maintain backward compatibility while enabling this flexibility, I added a Config field to the FlagConfig struct. This approach preserves the existing behavior while allowing external projects to inject configuration directly.

This allows the Serve-function to be called with out having a config file.
This makes the function more easilly callable from other projects. This way configuration is
not limited to an existing yaml file but can be specified in the project that is using this package.

Signed-off-by: peppi-lotta <[email protected]>
@peppi-lotta
Copy link
Author

@SuperQ Here's my proposed change to enhance FlagConfig so it can accept either a config file path or a Config object. This change maintains existing functionality, but makes the code easier to integrate from other codebases.

Let me know if this aligns with your vision for the refactor. I'm happy to adjust the implementation in any way if needed.

@kashifest
Copy link

@SuperQ can we have some review on this? It would be good to get this one rolling

return
}
} else {
c = u.config
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check that u.config is well-defined? The code later loops over c.HTTPConfig.Header and accesses c.Users[user]. getConfig() also sets some defaults here:

c := &Config{

}
} else {
// Use the provided config.
c = flags.WebConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here,

c := &Config{
sets also certain defaults.

@mrueg
Copy link
Contributor

mrueg commented Jul 14, 2025

Thanks for the PR! As right now this is either config struct or file based, do we see a use case where an app wants to set certain defaults via the config struct and have the user overwrite it via the file?

@SuperQ
Copy link
Member

SuperQ commented Jul 15, 2025

I think the either config or file is fine. We don't need a combo.

Signed-off-by: peppi-lotta <[email protected]>
@peppi-lotta
Copy link
Author

@mrueg I have now added a function for validating the config. Could you please take a look and provide a review when you have a chance?

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.

4 participants