Copilot commented on code in PR #13110:
URL: https://github.com/apache/trafficserver/pull/13110#discussion_r3122416400
##########
src/mgmt/config/ConfigRegistry.cc:
##########
@@ -460,9 +460,26 @@ ConfigRegistry::execute_reload(const std::string &key)
ctx.in_progress();
if (passed_config.IsDefined()) {
- // Passed config mode: store YAML node directly for handler to use via
supplied_yaml()
Dbg(dbg_ctl, "Config '%s' reloading from rpc-supplied content",
entry_copy.key.c_str());
- ctx.set_supplied_yaml(passed_config);
+
+ // Extract _reload directives before passing content to the handler.
+ // This keeps supplied_yaml() clean (pure config data) and provides
+ // reload_directives() as a separate accessor for operational parameters.
+ if (passed_config.IsMap() && passed_config["_reload"]) {
+ auto directives = passed_config["_reload"];
+ if (!directives.IsMap()) {
+ Warning("Config '%s': _reload must be a YAML map, ignoring
directives", entry_copy.key.c_str());
+ } else {
+ Dbg(dbg_ctl, "Config '%s' has reload directives",
entry_copy.key.c_str());
+ ctx.set_reload_directives(directives);
+ }
+ passed_config.remove("_reload");
Review Comment:
The `_reload` extraction check uses `passed_config["_reload"]` on a
non-const `YAML::Node`. In yaml-cpp, non-const `operator[]` *creates* the key
if it doesn’t exist, which makes the condition true and can trigger spurious
warnings/removals for configs that didn’t provide `_reload`. Use the const
`operator[]` (e.g. via `YAML::Node const &pc = passed_config;` then
`pc["_reload"]`) or another presence check that doesn’t mutate the node.
```suggestion
YAML::Node const &passed_config_view = passed_config;
if (passed_config.IsMap()) {
auto directives = passed_config_view["_reload"];
if (directives) {
if (!directives.IsMap()) {
Warning("Config '%s': _reload must be a YAML map, ignoring
directives", entry_copy.key.c_str());
} else {
Dbg(dbg_ctl, "Config '%s' has reload directives",
entry_copy.key.c_str());
ctx.set_reload_directives(directives);
}
passed_config.remove("_reload");
}
```
##########
src/mgmt/config/ConfigRegistry.cc:
##########
@@ -460,9 +460,26 @@ ConfigRegistry::execute_reload(const std::string &key)
ctx.in_progress();
if (passed_config.IsDefined()) {
- // Passed config mode: store YAML node directly for handler to use via
supplied_yaml()
Dbg(dbg_ctl, "Config '%s' reloading from rpc-supplied content",
entry_copy.key.c_str());
Review Comment:
`passed_config` is default-constructed as `YAML::Node`, and in this repo’s
vendored yaml-cpp `Node::IsDefined()` returns true even when `m_pNode ==
nullptr` (default ctor). As a result, `passed_config.IsDefined()` will be true
even when `_passed_configs` had no entry, so file-based reloads are
misclassified as RPC reloads (e.g. filename becomes ""). Consider initializing
`passed_config` as `YAML::Node{YAML::NodeType::Undefined}` and/or tracking a
`bool has_passed_config` from the map lookup instead of relying on
`IsDefined()` here.
##########
src/traffic_ctl/traffic_ctl.cc:
##########
@@ -164,6 +164,8 @@ main([[maybe_unused]] int argc, const char **argv)
// -d @- - read config from stdin
// -d "yaml: content" - inline yaml string
.add_option("--data", "-d", "Inline config data (@file, @- for stdin, or
yaml string)", "", MORE_THAN_ZERO_ARG_N, "")
+ .add_option("--directive", "-D", "Pass a reload directive to a config
handler (format: config_key.directive_key=value)", "",
+ MORE_THAN_ZERO_ARG_N, "")
Review Comment:
`--directive/-D` is registered with `MORE_THAN_ZERO_ARG_N`. With the current
ArgParser behavior, an option with “infinite args” consumes *all* remaining
argv entries, so any flags placed after `-D` will be swallowed as directive
values and likely cause confusing parse errors (same limitation as `-d`). If
you want `-D` to coexist with other flags, consider making `-D` take exactly 1
argument and allow repeating it (`-D a=b -D c=d`), or otherwise
document/enforce that `-D` must be last.
```suggestion
.add_option("--directive", "-D", "Pass a reload directive to a config
handler (format: config_key.directive_key=value)", "", 1,
"")
```
##########
include/mgmt/config/ConfigContext.h:
##########
@@ -177,12 +177,24 @@ class ConfigContext
/// @return copy of the supplied YAML node (cheap — YAML::Node is internally
reference-counted).
[[nodiscard]] YAML::Node supplied_yaml() const;
+ /// Get reload directives extracted from the _reload key.
+ /// Directives are operational parameters that modify how the handler
performs
+ /// the reload (e.g. scope to a single entry, dry-run) — distinct from
config content.
+ /// The framework extracts _reload from the supplied node before passing
content
+ /// to the handler, so supplied_yaml() never contains _reload.
+ /// @return copy of the directives YAML node (undefined if none were
provided).
+ [[nodiscard]] YAML::Node reload_directives() const;
Review Comment:
`ConfigContext::reload_directives()` is documented as returning an
“undefined” node when none were provided. With the vendored yaml-cpp,
default-constructed `YAML::Node` is `Null` but still `IsDefined()==true`, so
code like `if (auto directives = ctx.reload_directives())` won’t behave as
implied unless `_reload_directives` is explicitly initialized to
`YAML::NodeType::Undefined`. Please either update the implementation to ensure
the node is actually undefined when absent, or clarify the contract here (e.g.
require checking for specific keys via const lookups).
##########
doc/developer-guide/config-reload-framework.en.rst:
##########
@@ -266,8 +266,90 @@ supplied_yaml()
Returns the YAML node supplied via the RPC ``-d`` flag or ``configs``
parameter. If no inline
content was provided, the returned node is undefined (``operator bool()``
returns ``false``).
+ The framework strips the reserved ``_reload`` key from the supplied YAML
before delivering it
+ to the handler, so ``supplied_yaml()`` always contains pure config data.
+
+reload_directives()
+ Returns the YAML map extracted from the ``_reload`` key in the RPC-supplied
content. If no
+ directives were provided, the returned node is undefined (``operator
bool()`` returns ``false``).
+
+ Directives are operational parameters that modify **how** the handler
performs the reload —
+ they are distinct from config **content**. Common uses include scoping a
reload to a single
+ entry, enabling a dry-run mode, or passing a version constraint.
+
+ On the wire, directives are nested under ``_reload`` inside the handler's
``configs`` node:
+
+ .. code-block:: json
+
+ {
+ "configs": {
+ "myconfig": {
+ "_reload": { "id": "foo", "dry_run": "true" },
+ "rules": ["rule1", "rule2"]
+ }
+ }
+ }
+
+ The framework extracts ``_reload`` before the handler runs, so:
+
+ - ``reload_directives()`` returns ``{ "id": "foo", "dry_run": "true" }``
+ - ``supplied_yaml()`` returns the remaining content (without ``_reload``)
+ - If ``_reload`` was the only key, ``supplied_yaml()`` is undefined
+
Review Comment:
This section states that `reload_directives()` returns an “undefined” node
when no directives are provided (so `operator bool()` is false). With the
vendored yaml-cpp, a default-constructed `YAML::Node` is `Null` and
`IsDefined()==true`, so `if (auto directives = ctx.reload_directives())` will
be true even when nothing was provided unless `ConfigContext` explicitly
initializes `_reload_directives` to `NodeType::Undefined`. Either adjust the
implementation to match the documented contract, or update the docs to reflect
the actual truthiness semantics.
##########
doc/developer-guide/config-reload-framework.en.rst:
##########
@@ -266,8 +266,90 @@ supplied_yaml()
Returns the YAML node supplied via the RPC ``-d`` flag or ``configs``
parameter. If no inline
content was provided, the returned node is undefined (``operator bool()``
returns ``false``).
+ The framework strips the reserved ``_reload`` key from the supplied YAML
before delivering it
+ to the handler, so ``supplied_yaml()`` always contains pure config data.
+
+reload_directives()
+ Returns the YAML map extracted from the ``_reload`` key in the RPC-supplied
content. If no
+ directives were provided, the returned node is undefined (``operator
bool()`` returns ``false``).
+
+ Directives are operational parameters that modify **how** the handler
performs the reload —
+ they are distinct from config **content**. Common uses include scoping a
reload to a single
+ entry, enabling a dry-run mode, or passing a version constraint.
+
+ On the wire, directives are nested under ``_reload`` inside the handler's
``configs`` node:
+
+ .. code-block:: json
+
+ {
+ "configs": {
+ "myconfig": {
+ "_reload": { "id": "foo", "dry_run": "true" },
+ "rules": ["rule1", "rule2"]
+ }
+ }
+ }
+
+ The framework extracts ``_reload`` before the handler runs, so:
+
+ - ``reload_directives()`` returns ``{ "id": "foo", "dry_run": "true" }``
+ - ``supplied_yaml()`` returns the remaining content (without ``_reload``)
+ - If ``_reload`` was the only key, ``supplied_yaml()`` is undefined
+
+ Directives and content can coexist. The handler decides how to combine them
— the framework
+ delivers both without interpretation.
+
+ **Recommended handler pattern:**
+
+ .. code-block:: cpp
+
+ void MyConfig::reconfigure(ConfigContext ctx) {
+ ctx.in_progress();
+
+ if (auto directives = ctx.reload_directives()) {
+ if (directives["id"]) {
+ std::string id = directives["id"].as<std::string>();
+ if (!reload_single_entry(id)) {
+ ctx.fail("Unknown entry: " + id);
+ return;
+ }
+ ctx.complete("Reloaded entry: " + id);
+ return;
+ }
Review Comment:
The documentation’s recommended pattern uses `directives["id"]` on a
non-const `YAML::Node` (and relies on `operator bool()` for presence). With
yaml-cpp, non-const `operator[]` will create missing keys, so `if
(directives["id"])` can become true even when the directive wasn’t supplied,
and subsequent `.as<std::string>()` may throw. Prefer treating the node as
`const` before indexing (so missing keys return an undefined/zombie node), or
explicitly check `IsDefined()` on a const lookup result.
##########
src/traffic_ctl/CtrlCommands.cc:
##########
@@ -533,6 +560,20 @@ ConfigCommand::config_reload()
}
}
+ // Parse --directive (-D) arguments into configs[key]["_reload"][directive]
= value
+ auto dir_args = get_parsed_arguments()->get("directive");
+ for (auto const &dir : dir_args) {
+ if (dir.empty()) {
+ continue;
+ }
+ std::string err;
+ if (!parse_directive(dir, configs, err)) {
+ _printer->write_output("Error: " + err);
+ App_Exit_Status_Code = CTRL_EX_ERROR;
+ return;
Review Comment:
Similarly to the CLI option definition, parsing `--directive` values here
assumes the option parsing will only capture directive strings. Because `-D`
currently takes `MORE_THAN_ZERO_ARG_N`, any subsequent options can be captured
as directive args and end up producing a generic “Invalid directive format”
error. If keeping infinite-args semantics, consider adding a targeted error
when a directive arg starts with `-` (likely an option that got swallowed) to
make failures actionable.
##########
src/records/unit_tests/test_ReloadDirectives.cc:
##########
@@ -0,0 +1,318 @@
+/** @file
+
+ Unit tests for reload directives: ConfigContext directive accessors,
+ framework extraction logic, and CLI parse_directive() format.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+#include <catch2/catch_test_macros.hpp>
+
+#include "mgmt/config/ConfigContext.h"
+#include "mgmt/config/ConfigReloadTrace.h"
+
+#include <yaml-cpp/yaml.h>
+#include <string>
+#include <string_view>
+
+// ─── parse_directive: standalone copy of the traffic_ctl parsing logic
────────
+//
+// The actual function lives in an anonymous namespace in CtrlCommands.cc.
+// We reproduce the identical logic here so we can unit-test the format parsing
+// without pulling in the full traffic_ctl binary and its dependencies.
+namespace
+{
+bool
+parse_directive(std::string_view dir, YAML::Node &configs, std::string
&error_out)
+{
+ auto dot = dir.find('.');
+ if (dot == std::string_view::npos || dot == 0) {
+ error_out = "Invalid directive format '" + std::string(dir) + "'.
Expected: config_key.directive_key=value";
+ return false;
+ }
+
+ auto eq = dir.find('=', dot + 1);
+ if (eq == std::string_view::npos || eq == dot + 1) {
+ error_out = "Invalid directive format '" + std::string(dir) + "'.
Expected: config_key.directive_key=value";
+ return false;
+ }
+
+ std::string config_key{dir.substr(0, dot)};
+ std::string directive_key{dir.substr(dot + 1, eq - dot - 1)};
+ std::string value{dir.substr(eq + 1)};
+
+ configs[config_key]["_reload"][directive_key] = value;
+ return true;
+}
+} // namespace
+
+// ─── parse_directive format tests
─────────────────────────────────────────────
+
+TEST_CASE("parse_directive: valid single directive",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("myconfig.id=foo", configs, err));
+ REQUIRE(configs["myconfig"]["_reload"]["id"].as<std::string>() == "foo");
+}
+
+TEST_CASE("parse_directive: value with equals signs",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("plugin.url=http://x.com/a=b";, configs, err));
+ REQUIRE(configs["plugin"]["_reload"]["url"].as<std::string>() ==
"http://x.com/a=b";);
+}
+
+TEST_CASE("parse_directive: value with dots", "[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("plugin.fqdn=foo.example.com", configs, err));
+ REQUIRE(configs["plugin"]["_reload"]["fqdn"].as<std::string>() ==
"foo.example.com");
+}
+
+TEST_CASE("parse_directive: empty value is allowed",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("myconfig.flag=", configs, err));
+ REQUIRE(configs["myconfig"]["_reload"]["flag"].as<std::string>() == "");
+}
+
+TEST_CASE("parse_directive: multiple directives for same config",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("myconfig.id=foo", configs, err));
+ REQUIRE(parse_directive("myconfig.dry_run=true", configs, err));
+
+ REQUIRE(configs["myconfig"]["_reload"]["id"].as<std::string>() == "foo");
+ REQUIRE(configs["myconfig"]["_reload"]["dry_run"].as<std::string>() ==
"true");
+}
+
+TEST_CASE("parse_directive: multiple directives for different configs",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("myconfig.id=foo", configs, err));
+ REQUIRE(parse_directive("sni.fqdn=example.com", configs, err));
+
+ REQUIRE(configs["myconfig"]["_reload"]["id"].as<std::string>() == "foo");
+ REQUIRE(configs["sni"]["_reload"]["fqdn"].as<std::string>() ==
"example.com");
+}
+
+TEST_CASE("parse_directive: rejects missing dot", "[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE_FALSE(parse_directive("nodot", configs, err));
+ REQUIRE(err.find("Invalid directive format") != std::string::npos);
+}
+
+TEST_CASE("parse_directive: rejects leading dot", "[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE_FALSE(parse_directive(".key=value", configs, err));
+ REQUIRE(err.find("Invalid directive format") != std::string::npos);
+}
+
+TEST_CASE("parse_directive: rejects missing equals",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE_FALSE(parse_directive("config.key", configs, err));
+ REQUIRE(err.find("Invalid directive format") != std::string::npos);
+}
+
+TEST_CASE("parse_directive: rejects empty directive key",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE_FALSE(parse_directive("config.=value", configs, err));
+ REQUIRE(err.find("Invalid directive format") != std::string::npos);
+}
+
+// ─── ConfigContext directive accessor tests
───────────────────────────────────
+
+TEST_CASE("ConfigContext: reload_directives on default context has no keys",
"[config][context][directive]")
+{
+ ConfigContext ctx;
+
+ // Default-constructed YAML::Node is Null (IsDefined()==true).
+ // The handler pattern checks specific keys, which are undefined:
+ auto directives = ctx.reload_directives();
+ REQUIRE(directives.IsNull());
+ REQUIRE_FALSE(directives["id"].IsDefined());
+}
+
+TEST_CASE("ConfigContext: supplied_yaml on default context has no content",
"[config][context][directive]")
+{
+ ConfigContext ctx;
+
+ auto yaml = ctx.supplied_yaml();
+ REQUIRE(yaml.IsNull());
+ REQUIRE_FALSE(yaml.IsMap());
+ REQUIRE_FALSE(yaml.IsSequence());
+}
+
+TEST_CASE("ConfigContext: reload_directives round-trip via task",
"[config][context][directive]")
+{
+ auto task = std::make_shared<ConfigReloadTask>("test-dir-1",
"test", false, nullptr);
+ ConfigContext ctx(task, "test_handler");
+
+ YAML::Node directives;
+ directives["id"] = "foo";
+ directives["dry_run"] = "true";
+
+ // Use the private setter via a ConfigContext that has a live task.
+ // Since set_reload_directives is private and friend-accessible only from
+ // ConfigRegistry/ReloadCoordinator, we test through the public interface
+ // after setting up the state that execute_reload would create.
+
+ // Simulate what ConfigRegistry::execute_reload() does:
+ // Build a passed_config with _reload, then manually extract
+ YAML::Node passed_config;
+ passed_config["_reload"]["id"] = "foo";
+ passed_config["_reload"]["dry_run"] = "true";
+ passed_config["data"] = "some content";
+
+ // Extract _reload (same logic as execute_reload)
+ if (passed_config.IsMap() && passed_config["_reload"]) {
+ auto dir = passed_config["_reload"];
+ if (dir.IsMap()) {
+ // We can't call set_reload_directives directly (private).
+ // But we can verify the extraction logic works on the YAML node.
+ REQUIRE(dir["id"].as<std::string>() == "foo");
+ REQUIRE(dir["dry_run"].as<std::string>() == "true");
+ passed_config.remove("_reload");
+ }
+ }
+
+ // After extraction, passed_config should only have "data"
+ REQUIRE_FALSE(passed_config["_reload"].IsDefined());
+ REQUIRE(passed_config["data"].as<std::string>() == "some content");
+ REQUIRE(passed_config.size() == 1);
+}
+
+TEST_CASE("ConfigContext: _reload extraction with directives only",
"[config][context][directive]")
+{
+ YAML::Node passed_config;
+ passed_config["_reload"]["id"] = "bar";
+
+ // Extract
+ YAML::Node directives;
+ if (passed_config.IsMap() && passed_config["_reload"]) {
+ directives = passed_config["_reload"];
+ passed_config.remove("_reload");
+ }
+
+ REQUIRE(directives.IsDefined());
+ REQUIRE(directives["id"].as<std::string>() == "bar");
+
+ // After extraction with only _reload, the map should be empty
+ REQUIRE(passed_config.size() == 0);
+}
+
+TEST_CASE("ConfigContext: _reload extraction with content only",
"[config][context][directive]")
+{
+ YAML::Node passed_config;
+ passed_config["rules"].push_back("rule1");
+ passed_config["rules"].push_back("rule2");
+
+ bool extracted = false;
+ if (passed_config.IsMap() && passed_config["_reload"]) {
+ extracted = true;
+ passed_config.remove("_reload");
+ }
+
Review Comment:
Several tests use the pattern `if (passed_config.IsMap() &&
passed_config["_reload"])` on a non-const `YAML::Node`. In this repo’s
yaml-cpp, non-const `operator[]` inserts the key when missing, so this
condition will evaluate true even when `_reload` wasn’t present (and the
"content only" test will fail). Update these checks to use the const
`operator[]` (e.g. `YAML::Node const &pc = passed_config; if (auto dir =
pc["_reload"]; dir.IsDefined()) ...`) so presence checks don’t mutate the node
under test.
##########
doc/appendices/command-line/traffic_ctl.en.rst:
##########
@@ -414,6 +414,56 @@ Display the current value of a configuration record.
will return an error for the corresponding key. The JSONRPC response
will contain
per-key error details.
+ .. option:: --directive, -D <config_key.directive_key=value>
+
+ Pass a reload directive to a specific config handler. Directives are
operational parameters
+ that modify how the handler performs the reload — for example, scoping a
reload to a single
+ entry or enabling a dry-run mode. They are distinct from config content
(``-d``).
+
+ The format is ``config_key.directive_key=value``, parsed by splitting on
the first ``.``
+ and the first ``=``:
+
+ - ``config_key`` — the registry key (e.g. ``ip_allow``, ``sni``)
+ - ``directive_key`` — the directive name understood by that handler
+ - ``value`` — the directive value (always passed as a string on the wire)
+
+ Multiple directives are passed as space-separated values after a single
``-D``:
+
+ .. code-block:: bash
+
+ # Single directive
+ $ traffic_ctl config reload -D myconfig.id=foo
+
+ # Multiple directives for the same handler
+ $ traffic_ctl config reload -D myconfig.id=foo myconfig.dry_run=true
+
+ # Directives for different handlers in the same reload
+ $ traffic_ctl config reload -D myconfig.id=foo sni.fqdn=example.com
+
+ On the wire, ``-D myconfig.id=foo`` translates to:
Review Comment:
The `--directive/-D` docs describe multiple space-separated values after a
single `-D`, but with the current ArgParser behavior (infinite-arg options
consume the rest of argv) this also implies `-D` must be the last option on the
command line, otherwise subsequent flags are treated as directive values.
Consider documenting this ordering requirement explicitly, or changing `-D` to
take exactly one argument and allow repeating it to avoid this footgun.
##########
src/records/unit_tests/test_ReloadDirectives.cc:
##########
@@ -0,0 +1,318 @@
+/** @file
+
+ Unit tests for reload directives: ConfigContext directive accessors,
+ framework extraction logic, and CLI parse_directive() format.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+#include <catch2/catch_test_macros.hpp>
+
+#include "mgmt/config/ConfigContext.h"
+#include "mgmt/config/ConfigReloadTrace.h"
+
+#include <yaml-cpp/yaml.h>
+#include <string>
+#include <string_view>
+
+// ─── parse_directive: standalone copy of the traffic_ctl parsing logic
────────
+//
+// The actual function lives in an anonymous namespace in CtrlCommands.cc.
+// We reproduce the identical logic here so we can unit-test the format parsing
+// without pulling in the full traffic_ctl binary and its dependencies.
+namespace
+{
+bool
+parse_directive(std::string_view dir, YAML::Node &configs, std::string
&error_out)
+{
+ auto dot = dir.find('.');
+ if (dot == std::string_view::npos || dot == 0) {
+ error_out = "Invalid directive format '" + std::string(dir) + "'.
Expected: config_key.directive_key=value";
+ return false;
+ }
+
+ auto eq = dir.find('=', dot + 1);
+ if (eq == std::string_view::npos || eq == dot + 1) {
+ error_out = "Invalid directive format '" + std::string(dir) + "'.
Expected: config_key.directive_key=value";
+ return false;
+ }
+
+ std::string config_key{dir.substr(0, dot)};
+ std::string directive_key{dir.substr(dot + 1, eq - dot - 1)};
+ std::string value{dir.substr(eq + 1)};
+
+ configs[config_key]["_reload"][directive_key] = value;
+ return true;
+}
+} // namespace
+
+// ─── parse_directive format tests
─────────────────────────────────────────────
+
+TEST_CASE("parse_directive: valid single directive",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("myconfig.id=foo", configs, err));
+ REQUIRE(configs["myconfig"]["_reload"]["id"].as<std::string>() == "foo");
+}
+
+TEST_CASE("parse_directive: value with equals signs",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("plugin.url=http://x.com/a=b";, configs, err));
+ REQUIRE(configs["plugin"]["_reload"]["url"].as<std::string>() ==
"http://x.com/a=b";);
+}
+
+TEST_CASE("parse_directive: value with dots", "[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("plugin.fqdn=foo.example.com", configs, err));
+ REQUIRE(configs["plugin"]["_reload"]["fqdn"].as<std::string>() ==
"foo.example.com");
+}
+
+TEST_CASE("parse_directive: empty value is allowed",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("myconfig.flag=", configs, err));
+ REQUIRE(configs["myconfig"]["_reload"]["flag"].as<std::string>() == "");
+}
+
+TEST_CASE("parse_directive: multiple directives for same config",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("myconfig.id=foo", configs, err));
+ REQUIRE(parse_directive("myconfig.dry_run=true", configs, err));
+
+ REQUIRE(configs["myconfig"]["_reload"]["id"].as<std::string>() == "foo");
+ REQUIRE(configs["myconfig"]["_reload"]["dry_run"].as<std::string>() ==
"true");
+}
+
+TEST_CASE("parse_directive: multiple directives for different configs",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE(parse_directive("myconfig.id=foo", configs, err));
+ REQUIRE(parse_directive("sni.fqdn=example.com", configs, err));
+
+ REQUIRE(configs["myconfig"]["_reload"]["id"].as<std::string>() == "foo");
+ REQUIRE(configs["sni"]["_reload"]["fqdn"].as<std::string>() ==
"example.com");
+}
+
+TEST_CASE("parse_directive: rejects missing dot", "[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE_FALSE(parse_directive("nodot", configs, err));
+ REQUIRE(err.find("Invalid directive format") != std::string::npos);
+}
+
+TEST_CASE("parse_directive: rejects leading dot", "[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE_FALSE(parse_directive(".key=value", configs, err));
+ REQUIRE(err.find("Invalid directive format") != std::string::npos);
+}
+
+TEST_CASE("parse_directive: rejects missing equals",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE_FALSE(parse_directive("config.key", configs, err));
+ REQUIRE(err.find("Invalid directive format") != std::string::npos);
+}
+
+TEST_CASE("parse_directive: rejects empty directive key",
"[config][directive][parse]")
+{
+ YAML::Node configs;
+ std::string err;
+
+ REQUIRE_FALSE(parse_directive("config.=value", configs, err));
+ REQUIRE(err.find("Invalid directive format") != std::string::npos);
+}
+
+// ─── ConfigContext directive accessor tests
───────────────────────────────────
+
+TEST_CASE("ConfigContext: reload_directives on default context has no keys",
"[config][context][directive]")
+{
+ ConfigContext ctx;
+
+ // Default-constructed YAML::Node is Null (IsDefined()==true).
+ // The handler pattern checks specific keys, which are undefined:
+ auto directives = ctx.reload_directives();
+ REQUIRE(directives.IsNull());
+ REQUIRE_FALSE(directives["id"].IsDefined());
+}
+
+TEST_CASE("ConfigContext: supplied_yaml on default context has no content",
"[config][context][directive]")
+{
+ ConfigContext ctx;
+
+ auto yaml = ctx.supplied_yaml();
+ REQUIRE(yaml.IsNull());
+ REQUIRE_FALSE(yaml.IsMap());
+ REQUIRE_FALSE(yaml.IsSequence());
+}
+
+TEST_CASE("ConfigContext: reload_directives round-trip via task",
"[config][context][directive]")
+{
+ auto task = std::make_shared<ConfigReloadTask>("test-dir-1",
"test", false, nullptr);
+ ConfigContext ctx(task, "test_handler");
+
+ YAML::Node directives;
+ directives["id"] = "foo";
+ directives["dry_run"] = "true";
+
+ // Use the private setter via a ConfigContext that has a live task.
+ // Since set_reload_directives is private and friend-accessible only from
+ // ConfigRegistry/ReloadCoordinator, we test through the public interface
+ // after setting up the state that execute_reload would create.
+
+ // Simulate what ConfigRegistry::execute_reload() does:
+ // Build a passed_config with _reload, then manually extract
+ YAML::Node passed_config;
+ passed_config["_reload"]["id"] = "foo";
+ passed_config["_reload"]["dry_run"] = "true";
+ passed_config["data"] = "some content";
+
+ // Extract _reload (same logic as execute_reload)
+ if (passed_config.IsMap() && passed_config["_reload"]) {
+ auto dir = passed_config["_reload"];
+ if (dir.IsMap()) {
+ // We can't call set_reload_directives directly (private).
+ // But we can verify the extraction logic works on the YAML node.
+ REQUIRE(dir["id"].as<std::string>() == "foo");
+ REQUIRE(dir["dry_run"].as<std::string>() == "true");
+ passed_config.remove("_reload");
+ }
+ }
+
+ // After extraction, passed_config should only have "data"
+ REQUIRE_FALSE(passed_config["_reload"].IsDefined());
+ REQUIRE(passed_config["data"].as<std::string>() == "some content");
+ REQUIRE(passed_config.size() == 1);
+}
Review Comment:
This new unit test file mostly re-validates YAML node manipulations in
isolation, but it doesn’t exercise the actual integration path where
`ConfigRegistry::execute_reload()` extracts `_reload`, sets
`ctx.reload_directives()`, and strips `_reload` from `ctx.supplied_yaml()`.
Consider adding a test that registers a temporary `FileAndRpc` handler with
`ConfigRegistry`, seeds `_passed_configs` via `set_passed_config()`, runs
`execute_reload()`, and asserts the handler observes the expected
directives/content (and that child contexts inherit directives).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
