PMM-11826 Refactor vmagent configuration handling. #4049
Conversation
Moved VM configuration flags from command-line arguments to environment variables to enhance flexibility and system override. Added support for extracting credentials from external Victoria Metrics URLs. Updated test coverage for various credentials and environment scenarios.
BupycHuk
requested review from
idoqo,
JiriCtvrtka and
Copilot
and removed request for
a team
There was a problem hiding this comment.
Pull Request Overview
This PR refactors vmagent configuration handling by moving most flags into environment variables, introducing URL credential extraction, and updating tests to match the new env-based setup.
- Switch most vmagent settings from Args to Env with an
-envflagprefix and handle system‐provided VMAGENT_ variables. - Add
extractCredentialsFromURLto parse basic auth from external VM URLs and wire it intovmAgentConfig. - Update unit tests (
vmagent_test.goandsupervisor_test.go) to validate the new Env outputs and credential behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| managed/services/agents/vmagent.go | Refactored vmAgentConfig to use envflag, extract URL credentials, and build Env vars |
| managed/services/agents/vmagent_test.go | Updated tests to assert on actual.Env instead of actual.Args and cover new credential cases |
| agent/agents/supervisor/supervisor_test.go | Added a supervisor integration test for VM Agent reflecting updated Args/Env |
Comments suppressed due to low confidence (1)
managed/services/agents/vmagent_test.go:187
- The external VM tests exercise URL and credential extraction but don't verify that other defaults (e.g.
VMAGENT_remoteWrite_tlsInsecureSkipVerify,VMAGENT_promscrape_maxScrapeSize,VMAGENT_remoteWrite_maxDiskUsagePerURL,VMAGENT_loggerLevel) are present. Consider adding assertions to cover these default environment variables.
assert.Contains(t, actual.Env, "VMAGENT_remoteWrite_url="+expectedURL)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v3 #4049 +/- ##
==========================================
+ Coverage 43.63% 43.69% +0.05%
==========================================
Files 369 369
Lines 45225 45263 +38
==========================================
+ Hits 19735 19778 +43
+ Misses 23785 23778 -7
- Partials 1705 1707 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <[email protected]>
…urn types and improve variable initialization.
| @@ -31,48 +32,103 @@ var ( | |||
| maxScrapeSizeDefault = "64MiB" | |||
| ) | |||
|
|
|||
| // extractCredentialsFromURL extracts username and password from a URL string. | |||
There was a problem hiding this comment.
Are comments for not exposed func needed? I mena do we want to keep them? Since func naming is good and clear. Same applies for rest.
There was a problem hiding this comment.
it depends, but in this case AI just followed our codebase where we write comments for private functions. It's not necessary, hovewer doesn't harm.
PMM-11826
Link to the Feature Build: SUBMODULES-3939
This PR is made by AI under my control.
If this PR adds or removes or alters one or more API endpoints, please review and add or update the relevant API documents as well:
If this PR is related to some other PRs in this or other repositories, please provide links to those PRs: