Conversation
…ng the /config HTTP endpoint. Add pflags-generated config files and fix import paths to use non-versioned module. Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
EngHabu
force-pushed
the
enghabu/disable-config-ep
branch
from
f6d754f to
122892b
Compare
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
There was a problem hiding this comment.
Pull request overview
Introduces a prof configuration section for flytestdlib/profutils and uses it to optionally disable the /config HTTP endpoint exposed by the profiling/metrics server.
Changes:
- Add
profutils.ConfigwithDisableConfigEndpointand generated pflags support. - Make
/confighandler registration conditional onDisableConfigEndpoint. - Update/extend tests to account for the new
profconfig section in the config dump.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| flytestdlib/profutils/server.go | Uses prof config to conditionally register the /config endpoint and updates handler setup callsites. |
| flytestdlib/profutils/server_test.go | Updates handler registration in tests and extends expected config dump with the new prof section. |
| flytestdlib/profutils/config.go | Adds the prof config section and GetConfig() accessor. |
| flytestdlib/profutils/config_flags.go | Generated pflags for profutils.Config. |
| flytestdlib/profutils/config_flags_test.go | Generated tests for the pflags wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| configSection = config.MustRegisterSection(configSectionKey, defaultConfig) | ||
| defaultConfig = &Config{} |
There was a problem hiding this comment.
defaultConfig is referenced before it’s initialized: configSection is created using defaultConfig, but in a var (...) block Go initializes top-to-bottom, so defaultConfig is still nil at that point. config.MustRegisterSection rejects nil configs and will panic during package init. Reorder these declarations (initialize defaultConfig first) or split into separate var statements so defaultConfig is non-nil when registering the section.
| configSection = config.MustRegisterSection(configSectionKey, defaultConfig) | |
| defaultConfig = &Config{} | |
| defaultConfig = &Config{} | |
| configSection = config.MustRegisterSection(configSectionKey, defaultConfig) |
| ) | ||
|
|
||
| type Config struct { | ||
| DisableConfigEndpoint bool `config:"DisableConfigEndpoint"` |
There was a problem hiding this comment.
The config system in this repo decodes using json struct tags (see flytestdlib/config/viper/viper.go where mapstructure TagName is json), but this struct uses a config:"..." tag. As a result, config keys / dumped config use the Go field name (DisableConfigEndpoint) and the generated pflag is also DisableConfigEndpoint, which is inconsistent with other sections (e.g. logger uses json:"show-source"). Consider switching to a json:"..." tag (and optionally a pflag description) to match existing conventions and make config file/env/pflag naming consistent.
| DisableConfigEndpoint bool `config:"DisableConfigEndpoint"` | |
| DisableConfigEndpoint bool `json:"disable-config-endpoint" pflag:"disable-config-endpoint,Disable the /config endpoint"` |
| // 1. the prometheus HTTP handler on '/metrics' path shared with the profiling server. | ||
| // 2. A healthcheck (L7) handler on '/healthcheck'. | ||
| // 3. A version handler on '/version' provides information about the specific build. | ||
| // 4. A config handler on '/config' provides a dump of the currently loaded config. |
There was a problem hiding this comment.
The doc comment for StartProfilingServerWithDefaultHandlers says the /config handler is always registered, but it’s now conditional on DisableConfigEndpoint. Please update the comment to reflect that /config is only exposed when the config endpoint is enabled (or mention the flag/key controlling it).
| // 4. A config handler on '/config' provides a dump of the currently loaded config. | |
| // 4. A config handler on '/config' provides a dump of the currently loaded config, when the | |
| // config endpoint is enabled (i.e., when DisableConfigEndpoint is false). |
| if !cfg.DisableConfigEndpoint { | ||
| handlers[configPath] = http.HandlerFunc(configHandler) | ||
| } |
There was a problem hiding this comment.
New behavior: /config may be omitted when DisableConfigEndpoint is true, but there’s no test covering that path. Since this package already has handler registration tests via http.DefaultServeMux, please add a test that sets DisableConfigEndpoint=true, calls configureGlobalHTTPHandler, and verifies /config is not served (e.g. returns 404 / falls through). You may need to reset http.DefaultServeMux in the test to avoid interference from the package init() registration.
…ng the /config HTTP endpoint. Add pflags-generated config files and fix import paths to use non-versioned module. (#7015) * Add DisableConfigEndpoint option to profutils Config to allow disabling the /config HTTP endpoint. Add pflags-generated config files and fix import paths to use non-versioned module. Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * fix imports Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Fix tests Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Fix config registration Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> * Fix unit tests Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com> --------- Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
Tracking issue
Why are the changes needed?
What changes were proposed in this pull request?
How was this patch tested?
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
main