Skip to content

Add Config to FlagConfig struct - #332

Open
peppi-lotta wants to merge 3 commits into
prometheus:masterfrom
Nordix:peppi-lotta/add-web.Config-to-FlagConfig
Open

peppi-lotta wants to merge 3 commits into
prometheus:masterfrom
Nordix:peppi-lotta/add-web.Config-to-FlagConfig

Conversation

@peppi-lotta

Copy link
Copy Markdown

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.

@peppi-lotta

Copy link
Copy Markdown
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
Copy Markdown

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

Comment thread web/handler.go
Comment thread web/tls_config.go
@mrueg

mrueg commented Jul 14, 2025

Copy link
Copy Markdown
Contributor

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

SuperQ commented Jul 15, 2025

Copy link
Copy Markdown
Member

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

@peppi-lotta

Copy link
Copy Markdown
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?

@nuhakala

nuhakala commented Aug 5, 2025

Copy link
Copy Markdown

@mrueg Could we get a review on this?

@peppi-lotta

Copy link
Copy Markdown
Author

@mrueg @SuperQ Please let me know if there is something I can do to move this forward :)

@peppi-lotta
peppi-lotta force-pushed the peppi-lotta/add-web.Config-to-FlagConfig branch from 5560ca0 to 3c64144 Compare September 15, 2025 06:43
@peppi-lotta

peppi-lotta commented Sep 26, 2025 •

Copy link
Copy Markdown
Author

@mrueg @SuperQ Please review these changes or let me know who I can ping for review :)

@peppi-lotta
peppi-lotta force-pushed the peppi-lotta/add-web.Config-to-FlagConfig branch from 3c64144 to 583cf60 Compare September 14, 2026 09:41
@peppi-lotta

Copy link
Copy Markdown
Author

@ArthurSens @mrueg @SuperQ This has been on hiatus for a while but I feel this is still a good addition. PTAL :)

@ArthurSens

Copy link
Copy Markdown
Member

/approve-workflow

@ArthurSens ArthurSens left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure if we want WebConfig to override WebConfigFile. I'm happy for them to be mutually exclusive, then downstream projects using exporter-toolkit can implement the overriding logic by themselves. My worry is that today WebConfigFile is the only way of doing this, if the new WebConfig overrides the current behavior, that's a breaking behavior. Feels safer to not do the override 🤔

Comment thread web/handler.go Outdated
Comment thread web/tls_config.go Outdated
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 <peppi-lotta.saari@est.tech>
Signed-off-by: peppi-lotta <peppi-lotta.saari@est.tech>
Signed-off-by: peppi-lotta <peppi-lotta.saari@est.tech>
@peppi-lotta
peppi-lotta force-pushed the peppi-lotta/add-web.Config-to-FlagConfig branch from 45fa521 to c161759 Compare October 2, 2026 07:23
@peppi-lotta

Copy link
Copy Markdown
Author

I'm not sure if we want WebConfig to override WebConfigFile. I'm happy for them to be mutually exclusive, then downstream projects using exporter-toolkit can implement the overriding logic by themselves. My worry is that today WebConfigFile is the only way of doing this, if the new WebConfig overrides the current behavior, that's a breaking behavior. Feels safer to not do the override 🤔

I've now made WebConfig to override WebConfigFile mutually exclusive. If both are set there will be a "conflicting flags" error. I've added necessary unit testing for this as well.

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.

6 participants