feat(config-migrate): couple to infra-agent agent type, simplify by DavSanchez · Pull Request #1640 · newrelic/newrelic-agent-control

DavSanchez · 2025-09-10T12:40:23Z

What this PR does / why we need it

Follow-up for #1610 and #1619, this adapts the code for the config-migrate utility so it is coupled to our single use case (the newrelic-infra agent type) and performs its actions in the most straightforward way possible without rewriting the whole program.

I have added scenario testing with filesystem access mocked so you can check how this works. Please see converter.rs.

Remaining tasks after all the currently opened PRs are merged:

Document the new way to define filesystem items in INTEGRATING_AGENTS.md as it's promoted to the official one.
Remove file and map[string]file variable types and related structure (injected deps).
Increase the correctness of the rendering process by splitting the agent type definition type into two, an "unrendered" and a "rendered" one related via the rendering functions, only allowing inspection of final values for creating supervisors on the latter.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

Provided a meaningful title following conventional commit style.
Included a detailed description for the Pull Request.
Documentation under docs is aligned with the change.
Follows guidelines for Pull Requests in CONTRIBUTING.md.
Follows log level guidelines.

DavSanchez · 2025-09-11T00:32:13Z

E2E test for on-host migration scenario (part of nightly builds) is successful: https://github.com/newrelic/newrelic-agent-control/actions/runs/17630098266/job/50096362311

agent-control/src/config_migrate/migration/converter.rs

sigilioso

The approach looks good! Find some comment below

sigilioso · 2025-09-12T07:07:57Z

agent-control/src/config_migrate/migration/converter.rs

+        let file_reader = &self.file_reader;
+
+        // Retrieve the configuration for an existing infrastructure-agent installation.
+        let config_agent_key = String::from("config_agent");


Could we use constants for the keys? I also wonder if we could have some sort of sanity check if the keys ever change in the Agent Type? Maybe a test involving the registry could be enough 🤔

agent-control/src/config_migrate/migration/converter.rs

sigilioso · 2025-09-12T07:16:48Z

agent-control/src/config_migrate/migration/supported_config_value.rs

+                if let Some((key, value)) = map.next_entry::<String, Sequence>()? {
+                    if map.next_key::<String>()?.is_some() {
+                        Err(M::Error::custom(
+                            "expected a single key-value pair in the map",


Shouldn't we support a single config file with multiple keys?

integrations: - name: nri-docker # ... - name: nri-postgresql # ... discovery: # ... variables: # ...

I'm not completely sure if we were supporting it before (and I'm also not sure if the infra agent works well with such config), but the docs made me wonder.

You might be right. My goal was the simplest thing possible that works in our current setup, but it might make sense to open up for a bit more. Looking into it!

This highlighted the need of multiple files and now it is solved, right?

This was tied to expecting a certain file structure for integrations and logging files and how to merge them into a single file, but now it's not needed yes!

Besides, I think it was better to have it accept generic YAML instead of defining structures for these file types, given it might change by the OHAI team and we might not notice (introducing new fields or something), so I removed the custom parse.

agent-control/src/config_migrate/migration/defaults.rs

sigilioso

Thanks for taking care of this. Find some comments below.

sigilioso · 2025-10-01T09:52:51Z

agent-control/src/config_migrate/migration/converter.rs

+fn process_config_input(input: String) -> String {
+    // This regex matches {{VAR_NAME}} not already inside quotes
+    let re = Regex::new(r#"(?P<pre>[^'"]|^)\{\{([A-Za-z0-9_]+)\}\}(?P<post>[^'"]|$)"#).unwrap();
+    re.replace_all(&input, "${pre}'{{${2}}}'${post}")
+        .to_string()
+}


We are building the once for each function call, right? Could we build it only once? Maybe something similar to what we do here:

newrelic-agent-control/agent-control/src/agent_type/templates_function.rs

Lines 10 to 13 in 18c6368

fn template_functions_re() -> &'static Regex {

static RE_ONCE: OnceLock<Regex> = OnceLock::new();

RE_ONCE.get_or_init(|| Regex::new(TEMPLATE_PIPE_FUNCTIONS).unwrap())

}

Aah yes, makes total sense! I'll add it.

sigilioso · 2025-10-01T09:56:28Z

agent-control/src/config_migrate/migration/converter.rs

+/// This is a regex-based approach that may not cover all edge cases, but works for
+/// the common scenarios we expect (small config strings).


Do you have any of those edge cases in mind? 🤔
I was thinking of values that are not supposed to be a string in the config, but I'm not sure there could be any.

I don't think we need to do it now but I think we should allocate some time to document the migration limitations.

Not sure what would be cases that should work for the infra-agent but not in our migration. In all honesty I wrote the comment when I had another implementation (manual successive replacement of occurrences with String -> String functions) so it probably had flaws that the current implementation hasn't, so I'll reword the comments and start writing docs for the migration tool, if only in the top-level module.

Probably these docs should have their own task (I don't think it is in the scope of this change)

Reworded for now 2a8e204

sigilioso · 2025-10-01T10:06:06Z

agent-control/src/config_migrate/migration/converter.rs

+            if !visited.insert(k) {
+                Err(ConversionError::DuplicateKeyFound(k.clone()))


I'm not sure if it is feasible but the error message could really help understanding where the problem is, right? It currently says duplicate key found in file and dir mappings: key", but I think it would be useful to say where the key is (unless I am not getting it right!).

By the way, do you think that adding a unit-test to check this error would be worth it?

Done! 81754d9

As for specifying where exactly the error occurs, this is something that might happen in the config (the stand-alone structure in defaults.rs) that before was controlled only by us (the configs define files_map and dirs_map and if these both define the same key it will modify the migration behavior) but you're right that now I accept an external config input. I'll try to be more verbose to help... or rewrite this config so this cannot even happen 😆

Ok, I have removed the possibility of duplicate mapping keys altogether, so this shouldn't be an issue anymore, let me know your thoughts! d62334c

As for specifying where exactly the error occurs, this is something that might happen in the config (the stand-alone structure in defaults.rs)

I wasn't getting it right! I thought it were keys in the config files (which doesn't make sense because we can have duplicates there! (integrations might be defined many times!) 🤦

Anyway, I think the new changes simplify it quite a bit! 🚀

sigilioso

LGTM 🚀

The base branch was changed.

DavSanchez marked this pull request as ready for review September 10, 2025 15:04

DavSanchez requested a review from a team as a code owner September 10, 2025 15:04

DavSanchez force-pushed the refactor/migrate-host-agent-types-to-filesystem branch from 7ccec20 to 058494c Compare September 10, 2025 23:56

DavSanchez force-pushed the feat/adapt-config-migrate-util-to-filesystem branch from 73af478 to a82242b Compare September 11, 2025 00:08

DavSanchez force-pushed the feat/adapt-config-migrate-util-to-filesystem branch 2 times, most recently from a3ae80a to 85ec477 Compare September 11, 2025 00:42

DavSanchez added the onhost label Sep 11, 2025

sigilioso reviewed Sep 12, 2025

View reviewed changes

agent-control/src/config_migrate/migration/converter.rs Outdated Show resolved Hide resolved

sigilioso reviewed Sep 12, 2025

View reviewed changes

DavSanchez force-pushed the refactor/migrate-host-agent-types-to-filesystem branch 2 times, most recently from 1686105 to 94e58ee Compare September 15, 2025 07:39

DavSanchez force-pushed the feat/adapt-config-migrate-util-to-filesystem branch from 85ec477 to 79a2ea5 Compare September 15, 2025 08:31

DavSanchez force-pushed the refactor/migrate-host-agent-types-to-filesystem branch from ec82968 to d217e84 Compare September 15, 2025 08:31

DavSanchez force-pushed the feat/adapt-config-migrate-util-to-filesystem branch from 79a2ea5 to 36dce86 Compare September 15, 2025 08:32

DavSanchez force-pushed the refactor/migrate-host-agent-types-to-filesystem branch from d217e84 to 5bbf894 Compare September 15, 2025 08:43

DavSanchez force-pushed the feat/adapt-config-migrate-util-to-filesystem branch from 36dce86 to e6edf11 Compare September 15, 2025 08:44

DavSanchez marked this pull request as draft September 16, 2025 12:54

DavSanchez mentioned this pull request Sep 19, 2025

feat: directory support, map[string]yaml var type, migration #1685

Merged

5 tasks

DavSanchez changed the base branch from refactor/migrate-host-agent-types-to-filesystem to feat/vartype-map-string-yaml September 19, 2025 18:26

DavSanchez force-pushed the feat/adapt-config-migrate-util-to-filesystem branch 3 times, most recently from 1db5aba to 0985366 Compare September 19, 2025 18:35

DavSanchez force-pushed the feat/vartype-map-string-yaml branch from ffc1b8f to f251eb1 Compare September 23, 2025 11:02

DavSanchez force-pushed the feat/adapt-config-migrate-util-to-filesystem branch 3 times, most recently from bece22b to 3c55a30 Compare September 23, 2025 14:36

DavSanchez force-pushed the feat/vartype-map-string-yaml branch 2 times, most recently from 3ad1536 to 8383858 Compare September 25, 2025 13:58

DavSanchez force-pushed the feat/adapt-config-migrate-util-to-filesystem branch from 3c55a30 to cf0b832 Compare September 25, 2025 15:27

sigilioso reviewed Oct 1, 2025

View reviewed changes

DavSanchez force-pushed the feat/adapt-config-migrate-util-to-filesystem branch 3 times, most recently from 95ba5ce to d62334c Compare October 1, 2025 14:29

sigilioso previously approved these changes Oct 2, 2025

View reviewed changes

Base automatically changed from feat/vartype-map-string-yaml to main October 2, 2025 07:42

DavSanchez enabled auto-merge (squash) October 2, 2025 07:42

sigilioso previously approved these changes Oct 2, 2025

View reviewed changes

DavSanchez dismissed sigilioso’s stale review via 7fa1adb October 2, 2025 08:05

DavSanchez added 16 commits October 2, 2025 09:10

feat(config-migrate): couple to infra-agent type, simplify

3924bf5

style: error quoting

e34eec2

wip: redefining

2ca6e7d

feat: merge file and directory mappings

dc40c5a

test: fix

9e5937e

docs: clarify

58f018d

style: use only the filename for map keys

ee85b9d

chore: remove unused code

c1545fd

docs: remove misleading comment

0bd130b

feat: handle env var syntax for infra configs

5466038

docs: remove comment related to previous implementation

730f00a

style: compile Regex only once

38a588a

test: duplicate keys error

13888fc

docs: wording

74338fe

refactor: remove the possibility of duplicate mapping keys

ee89e21

style: fmt

b81090f

DavSanchez force-pushed the feat/adapt-config-migrate-util-to-filesystem branch from 7fa1adb to b81090f Compare October 2, 2025 08:11

sigilioso approved these changes Oct 2, 2025

View reviewed changes

DavSanchez merged commit 52f2aab into main Oct 2, 2025
26 checks passed

DavSanchez deleted the feat/adapt-config-migrate-util-to-filesystem branch October 2, 2025 08:30

	fn template_functions_re() -> &'static Regex {
	static RE_ONCE: OnceLock<Regex> = OnceLock::new();
	RE_ONCE.get_or_init(\|\| Regex::new(TEMPLATE_PIPE_FUNCTIONS).unwrap())
	}

		/// This is a regex-based approach that may not cover all edge cases, but works for
		/// the common scenarios we expect (small config strings).

		if !visited.insert(k) {
		Err(ConversionError::DuplicateKeyFound(k.clone()))

Conversation

DavSanchez commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it

Checklist

Uh oh!

DavSanchez commented Sep 11, 2025

Uh oh!

Uh oh!

sigilioso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sigilioso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavSanchez Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavSanchez Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigilioso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

DavSanchez commented Sep 10, 2025 •

edited

Loading

DavSanchez Oct 1, 2025 •

edited

Loading

DavSanchez Oct 1, 2025 •

edited

Loading