fix(gateway): pass plugin config from --params-file YAML to plugins#350
Open
fix(gateway): pass plugin config from --params-file YAML to plugins#350
Conversation
…t passed to plugins extract_plugin_config() reads from get_node_options().parameter_overrides(), which is always empty for --params-file YAML params. These go into the ROS 2 global rcl context instead. Plugins always receive empty config and fall back to defaults. The test inits rclcpp with --params-file containing plugin config, creates a GatewayNode with default NodeOptions (production path), and asserts that the YAML plugin params are accessible. Currently fails with: "Total overrides: 0" Closes #349
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new gateway unit test intended to reproduce/lock in the reported bug where plugin configuration provided via --params-file YAML is not propagated to plugins (because the current implementation reads from NodeOptions::parameter_overrides(), which stays empty in that launch path).
Changes:
- Add
test_plugin_config.cppto simulaterclcpp::init(... --params-file ...)and check plugin config visibility. - Register the new
test_plugin_configgtest target inros2_medkit_gatewayCMake.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/ros2_medkit_gateway/test/test_plugin_config.cpp |
New test attempting to prove the --params-file plugin-config propagation bug. |
src/ros2_medkit_gateway/CMakeLists.txt |
Adds test_plugin_config target and assigns it a unique ROS domain via medkit_set_test_domain. |
Comment on lines
+108
to
+125
| // Verify: parameter_overrides() should contain the plugin params. | ||
| // BUG: --params-file YAML params go to the global rcl context, | ||
| // NOT to NodeOptions::parameter_overrides(). | ||
| const auto & overrides = node->get_node_options().parameter_overrides(); | ||
| bool found_plugin_config = false; | ||
| for (const auto & p : overrides) { | ||
| if (p.get_name().rfind("plugins.test_plugin.", 0) == 0 && p.get_name() != "plugins.test_plugin.path") { | ||
| found_plugin_config = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // This assertion proves the bug: YAML plugin params are NOT in parameter_overrides | ||
| EXPECT_TRUE(found_plugin_config) | ||
| << "Plugin config from --params-file not found in parameter_overrides(). " | ||
| << "extract_plugin_config() reads from this source, so plugins receive empty config. " | ||
| << "Total overrides: " << overrides.size(); | ||
|
|
There was a problem hiding this comment.
The test asserts that plugin params from --params-file should appear in node->get_node_options().parameter_overrides(), but parameter_overrides() is only for programmatic NodeOptions::parameter_overrides(...) and is not expected to include params loaded from the global context. Even with a correct fix (changing extract_plugin_config() to read resolved overrides / declared parameters), parameter_overrides() may legitimately remain empty, so this expectation will keep failing and hard-codes an internal implementation detail. Prefer asserting the user-visible behavior (the plugin’s configure() receives the JSON config) or assert against the same resolved parameter source the gateway will use after the fix.
Comment on lines
+100
to
+103
| // Init rclcpp with --params-file (simulates production: ros2 run ... --ros-args --params-file ...) | ||
| const char * args[] = {"test_plugin_config", "--ros-args", "--params-file", yaml_path.c_str()}; | ||
| rclcpp::init(4, const_cast<char **>(args)); | ||
|
|
There was a problem hiding this comment.
rclcpp::init expects char **argv, but this passes a const char*[] and uses const_cast<char **> to satisfy the signature. This is unsafe if the ROS argument parsing code mutates argv contents. Build a mutable argv array (e.g., std::vector<std::string> + std::vector<char*> pointing to writable buffers) or use an rclcpp::Context with init that takes rclcpp::InitOptions if available.
Comment on lines
+44
to
+65
| int reserve_free_port() { | ||
| int sock = socket(AF_INET, SOCK_STREAM, 0); | ||
| if (sock < 0) { | ||
| return 0; | ||
| } | ||
| struct sockaddr_in addr {}; | ||
| addr.sin_family = AF_INET; | ||
| addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK); | ||
| addr.sin_port = 0; | ||
| if (bind(sock, reinterpret_cast<struct sockaddr *>(&addr), sizeof(addr)) < 0) { | ||
| close(sock); | ||
| return 0; | ||
| } | ||
| socklen_t len = sizeof(addr); | ||
| if (getsockname(sock, reinterpret_cast<struct sockaddr *>(&addr), &len) < 0) { | ||
| close(sock); | ||
| return 0; | ||
| } | ||
| int port = ntohs(addr.sin_port); | ||
| close(sock); | ||
| return port; | ||
| } |
There was a problem hiding this comment.
reserve_free_port() here differs from the existing helper used in other gateway tests (e.g. test_gateway_node.cpp) by not setting SO_REUSEADDR. That can make port reservation/binding more fragile in CI. Consider reusing the same helper implementation (including setsockopt(...SO_REUSEADDR...)) to reduce flakiness and keep behavior consistent across tests.
Comment on lines
+23
to
+39
| #include <gtest/gtest.h> | ||
| #include <httplib.h> // NOLINT(build/include_order) | ||
|
|
||
| #include <arpa/inet.h> | ||
| #include <cstdio> | ||
| #include <fstream> | ||
| #include <memory> | ||
| #include <netinet/in.h> | ||
| #include <string> | ||
| #include <sys/socket.h> | ||
| #include <thread> | ||
| #include <unistd.h> | ||
|
|
||
| #include <ament_index_cpp/get_package_prefix.hpp> | ||
| #include <nlohmann/json.hpp> | ||
| #include <rclcpp/rclcpp.hpp> | ||
|
|
There was a problem hiding this comment.
Several headers appear unused in this test (<httplib.h>, <nlohmann/json.hpp>, <thread>, potentially others). Removing unused includes will speed up builds and reduce accidental coupling to unrelated headers.
Comment on lines
+84
to
+141
| // Write YAML with plugin config | ||
| std::string yaml_path = "/tmp/test_plugin_config_" + std::to_string(getpid()) + ".yaml"; | ||
| { | ||
| std::ofstream yaml(yaml_path); | ||
| yaml << "ros2_medkit_gateway:\n" | ||
| << " ros__parameters:\n" | ||
| << " server:\n" | ||
| << " host: \"127.0.0.1\"\n" | ||
| << " port: " << free_port << "\n" | ||
| << " plugins: [\"test_plugin\"]\n" | ||
| << " plugins.test_plugin.path: \"" << test_plugin_path() << "\"\n" | ||
| << " plugins.test_plugin.custom_key: \"custom_value\"\n" | ||
| << " plugins.test_plugin.mode: \"testing\"\n" | ||
| << " plugins.test_plugin.nested.setting: 42\n"; | ||
| } | ||
|
|
||
| // Init rclcpp with --params-file (simulates production: ros2 run ... --ros-args --params-file ...) | ||
| const char * args[] = {"test_plugin_config", "--ros-args", "--params-file", yaml_path.c_str()}; | ||
| rclcpp::init(4, const_cast<char **>(args)); | ||
|
|
||
| // Create node WITHOUT plugin config in parameter_overrides (simulates main.cpp) | ||
| rclcpp::NodeOptions options; | ||
| auto node = std::make_shared<ros2_medkit_gateway::GatewayNode>(options); | ||
|
|
||
| // Verify: parameter_overrides() should contain the plugin params. | ||
| // BUG: --params-file YAML params go to the global rcl context, | ||
| // NOT to NodeOptions::parameter_overrides(). | ||
| const auto & overrides = node->get_node_options().parameter_overrides(); | ||
| bool found_plugin_config = false; | ||
| for (const auto & p : overrides) { | ||
| if (p.get_name().rfind("plugins.test_plugin.", 0) == 0 && p.get_name() != "plugins.test_plugin.path") { | ||
| found_plugin_config = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // This assertion proves the bug: YAML plugin params are NOT in parameter_overrides | ||
| EXPECT_TRUE(found_plugin_config) | ||
| << "Plugin config from --params-file not found in parameter_overrides(). " | ||
| << "extract_plugin_config() reads from this source, so plugins receive empty config. " | ||
| << "Total overrides: " << overrides.size(); | ||
|
|
||
| // Sanity check: the params DO exist in the global context (they're just not in overrides). | ||
| // Declaring them proves the YAML was parsed correctly. | ||
| try { | ||
| node->declare_parameter("plugins.test_plugin.custom_key", std::string("default")); | ||
| auto val = node->get_parameter("plugins.test_plugin.custom_key").as_string(); | ||
| EXPECT_EQ(val, "custom_value") << "YAML param exists in global context but not in parameter_overrides()"; | ||
| } catch (const rclcpp::exceptions::ParameterAlreadyDeclaredException &) { | ||
| // If already declared (e.g. by a fix), that's fine - get it directly | ||
| auto val = node->get_parameter("plugins.test_plugin.custom_key").as_string(); | ||
| EXPECT_EQ(val, "custom_value"); | ||
| } | ||
|
|
||
| node.reset(); | ||
| rclcpp::shutdown(); | ||
| std::remove(yaml_path.c_str()); | ||
| } |
There was a problem hiding this comment.
The YAML file is written to a fixed /tmp/... path and removed only at the end of the test. If an exception is thrown after rclcpp::init() (e.g., node construction failure), rclcpp::shutdown() and file cleanup may be skipped. Consider using a small RAII guard/scope-exit for rclcpp::shutdown() + file removal, and create the temp file via mkstemp/std::filesystem::temp_directory_path() to avoid collisions.
Comment on lines
+108
to
+137
| // Verify: parameter_overrides() should contain the plugin params. | ||
| // BUG: --params-file YAML params go to the global rcl context, | ||
| // NOT to NodeOptions::parameter_overrides(). | ||
| const auto & overrides = node->get_node_options().parameter_overrides(); | ||
| bool found_plugin_config = false; | ||
| for (const auto & p : overrides) { | ||
| if (p.get_name().rfind("plugins.test_plugin.", 0) == 0 && p.get_name() != "plugins.test_plugin.path") { | ||
| found_plugin_config = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // This assertion proves the bug: YAML plugin params are NOT in parameter_overrides | ||
| EXPECT_TRUE(found_plugin_config) | ||
| << "Plugin config from --params-file not found in parameter_overrides(). " | ||
| << "extract_plugin_config() reads from this source, so plugins receive empty config. " | ||
| << "Total overrides: " << overrides.size(); | ||
|
|
||
| // Sanity check: the params DO exist in the global context (they're just not in overrides). | ||
| // Declaring them proves the YAML was parsed correctly. | ||
| try { | ||
| node->declare_parameter("plugins.test_plugin.custom_key", std::string("default")); | ||
| auto val = node->get_parameter("plugins.test_plugin.custom_key").as_string(); | ||
| EXPECT_EQ(val, "custom_value") << "YAML param exists in global context but not in parameter_overrides()"; | ||
| } catch (const rclcpp::exceptions::ParameterAlreadyDeclaredException &) { | ||
| // If already declared (e.g. by a fix), that's fine - get it directly | ||
| auto val = node->get_parameter("plugins.test_plugin.custom_key").as_string(); | ||
| EXPECT_EQ(val, "custom_value"); | ||
| } | ||
|
|
There was a problem hiding this comment.
This test introduces a deliberately failing assertion (EXPECT_TRUE(found_plugin_config)) and will cause the ros2_medkit_gateway test suite to fail in CI if merged as-is. Please include the production fix in the same PR (or otherwise ensure the new test passes) so the repository stays green.
Suggested change
| // Verify: parameter_overrides() should contain the plugin params. | |
| // BUG: --params-file YAML params go to the global rcl context, | |
| // NOT to NodeOptions::parameter_overrides(). | |
| const auto & overrides = node->get_node_options().parameter_overrides(); | |
| bool found_plugin_config = false; | |
| for (const auto & p : overrides) { | |
| if (p.get_name().rfind("plugins.test_plugin.", 0) == 0 && p.get_name() != "plugins.test_plugin.path") { | |
| found_plugin_config = true; | |
| break; | |
| } | |
| } | |
| // This assertion proves the bug: YAML plugin params are NOT in parameter_overrides | |
| EXPECT_TRUE(found_plugin_config) | |
| << "Plugin config from --params-file not found in parameter_overrides(). " | |
| << "extract_plugin_config() reads from this source, so plugins receive empty config. " | |
| << "Total overrides: " << overrides.size(); | |
| // Sanity check: the params DO exist in the global context (they're just not in overrides). | |
| // Declaring them proves the YAML was parsed correctly. | |
| try { | |
| node->declare_parameter("plugins.test_plugin.custom_key", std::string("default")); | |
| auto val = node->get_parameter("plugins.test_plugin.custom_key").as_string(); | |
| EXPECT_EQ(val, "custom_value") << "YAML param exists in global context but not in parameter_overrides()"; | |
| } catch (const rclcpp::exceptions::ParameterAlreadyDeclaredException &) { | |
| // If already declared (e.g. by a fix), that's fine - get it directly | |
| auto val = node->get_parameter("plugins.test_plugin.custom_key").as_string(); | |
| EXPECT_EQ(val, "custom_value"); | |
| } | |
| // Verify the current ROS 2 behavior: parameters loaded via --params-file are | |
| // available on the node through the parameter API, but are not necessarily | |
| // mirrored into NodeOptions::parameter_overrides(). | |
| const auto & overrides = node->get_node_options().parameter_overrides(); | |
| bool found_plugin_config_in_overrides = false; | |
| for (const auto & p : overrides) { | |
| if (p.get_name().rfind("plugins.test_plugin.", 0) == 0 && p.get_name() != "plugins.test_plugin.path") { | |
| found_plugin_config_in_overrides = true; | |
| break; | |
| } | |
| } | |
| EXPECT_FALSE(found_plugin_config_in_overrides) | |
| << "Parameters loaded from --params-file should not be required to appear in " | |
| << "NodeOptions::parameter_overrides(); validate access via node parameters instead. " | |
| << "Total overrides: " << overrides.size(); | |
| // Sanity check: the params do exist on the node. Declaring them proves the | |
| // YAML was parsed correctly and made available through the parameter system. | |
| try { | |
| node->declare_parameter("plugins.test_plugin.custom_key", std::string("default")); | |
| node->declare_parameter("plugins.test_plugin.mode", std::string("default_mode")); | |
| node->declare_parameter("plugins.test_plugin.nested.setting", 0); | |
| } catch (const rclcpp::exceptions::ParameterAlreadyDeclaredException &) { | |
| // If already declared (e.g. by a future production fix), that's fine. | |
| } | |
| auto custom_key = node->get_parameter("plugins.test_plugin.custom_key").as_string(); | |
| EXPECT_EQ(custom_key, "custom_value") | |
| << "YAML plugin parameter should be accessible via the node parameter API"; | |
| auto mode = node->get_parameter("plugins.test_plugin.mode").as_string(); | |
| EXPECT_EQ(mode, "testing") | |
| << "Additional YAML plugin parameters should also be accessible via the node parameter API"; | |
| auto nested_setting = node->get_parameter("plugins.test_plugin.nested.setting").as_int(); | |
| EXPECT_EQ(nested_setting, 42) | |
| << "Nested YAML plugin parameters should be accessible via the node parameter API"; |
…lobal args extract_plugin_config() read from NodeOptions::parameter_overrides() which is always empty for --params-file YAML params (they go to the rcl global context instead). Plugins received empty config and fell back to defaults. Fix: declare_plugin_params_from_yaml() accesses rcl_arguments_get_param_overrides() from the global context, finds params matching the plugin prefix, and declares them on the node with their typed values. extract_plugin_config() then reads them via list_parameters()/get_parameter(). The NodeOptions::parameter_overrides path is preserved as the primary source for programmatic overrides (used in unit tests).
… test The previous test created a full GatewayNode which hung on Humble/Rolling CI due to heavy DDS/HTTP initialization. Replace with a plain rclcpp::Node that directly tests the declare_plugin_params_from_yaml() logic: 1. Writes YAML with plugin params (string, int, bool, double) 2. Inits rclcpp with --params-file 3. Confirms parameter_overrides() is empty (proves the bug) 4. Calls declare_plugin_params_from_yaml() and verifies all types resolve Test runs in ~50ms instead of timing out.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
Fix
extract_plugin_config()which reads fromget_node_options().parameter_overrides()- a collection that is always empty when params come from--params-file. Plugin config from YAML is never passed to plugins; they always fall back to defaults.This PR currently contains only a failing test that proves the bug. The fix will be added in a follow-up commit.
Issue
Type
Testing
test_plugin_config(PluginConfig.YamlPluginParamsReachGateway):plugins.test_plugin.custom_key: "custom_value"--params-file(simulates production launch)GatewayNodewith defaultNodeOptions(production code path)parameter_overrides()- currently fails withTotal overrides: 0Checklist