Copilot commented on code in PR #13853:
URL: https://github.com/apache/trafficserver/pull/13853#discussion_r4312898881
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -261,7 +307,22 @@ NextHopStrategyFactory::loadConfigFile(const std::string
&fileName, std::strings
}
const auto include_line = line_view.substr(include_start);
const auto include_end =
include_line.find_first_of(separators);
- const std::string f{include_line.substr(0, include_end)};
+ const std::string include_target{include_line.substr(0,
include_end)};
+ std::string f =
RecConfigReadConfigPath(nullptr, include_target.c_str());
+ bool used_legacy_path = false;
+
+ // Retry a missing include when it starts with the configuration
directory's old
+ // install-prefix-relative spelling, e.g.
etc/trafficserver/hosts.yaml.
+ struct stat include_stat;
+ if (stat(f.c_str(), &include_stat) == -1 && errno == ENOENT) {
+ std::string legacy_path =
legacy_config_dir_include_path(include_target);
+ struct stat legacy_stat;
+ if (!legacy_path.empty() && stat(legacy_path.c_str(),
&legacy_stat) == 0) {
Review Comment:
This code uses `struct stat`, `stat()`, `errno`, and `ENOENT` but the
file-level includes shown in the diff do not explicitly include the standard
headers that declare them. Relying on transitive includes is brittle and can
break builds. Add the appropriate includes (typically `<sys/stat.h>` and
`<cerrno>`) in this translation unit.
##########
tests/gold_tests/next_hop/strategies_include/strategies_include.test.py:
##########
@@ -0,0 +1,103 @@
+'''
+Verify '#include' handling in strategies.yaml.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
Review Comment:
The entire file appears to be indented at top-level (e.g., the module
docstring on line 1). In Python, leading indentation at top-level will raise
`IndentationError: unexpected indent`. Remove the leading spaces so all
top-level statements start at column 0.
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -27,16 +27,61 @@
*/
#include <catch2/catch_test_macros.hpp> /* catch unit-test framework */
-#include <fstream> /* ofstream */
+#include <catch2/reporters/catch_reporter_event_listener.hpp>
+#include <catch2/reporters/catch_reporter_registrars.hpp>
+#include <chrono>
+#include <cstdlib>
+#include <filesystem>
+#include <fstream> /* ofstream */
#include <memory>
#include <utime.h>
#include <yaml-cpp/yaml.h>
-#include "nexthop_test_stubs.h"
+#include "tscore/Layout.h"
#include "proxy/http/remap/NextHopSelectionStrategy.h"
#include "proxy/http/remap/NextHopStrategyFactory.h"
#include "proxy/http/remap/NextHopConsistentHash.h"
-#include "proxy/http/remap/NextHopRoundRobin.h"
+
+// '#include' directives in strategy files resolve against the configuration
+// directory, so point it at a temporary configuration directory containing the
+// test fixtures.
+static std::filesystem::path config_root;
+
+struct ConfigDirListener : Catch::EventListenerBase {
+ using EventListenerBase::EventListenerBase;
+
+ void
+ testRunStarting(Catch::TestRunInfo const & /* testRunInfo ATS_UNUSED */)
override
+ {
+ const auto temp_dir = std::filesystem::temp_directory_path();
+ const auto prefix = "nh_strategy_config_test-" +
std::to_string(std::chrono::steady_clock::now().time_since_epoch().count());
+
+ for (unsigned int attempt = 0; attempt < 100; ++attempt) {
+ config_root = temp_dir / (prefix + "-" + std::to_string(attempt));
+ if (std::filesystem::create_directory(config_root)) {
+ break;
+ }
+ config_root.clear();
+ }
+ if (config_root.empty()) {
+ std::abort();
+ }
Review Comment:
`std::abort()` makes failures hard to diagnose in CI (no assertion message)
and can bypass normal test reporting/cleanup. Prefer failing the test with
Catch2 (e.g., `FAIL(...)`/`REQUIRE(...)`) or throwing a descriptive exception
so the test output explains why the temp config directory could not be created.
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -271,6 +332,10 @@ NextHopStrategyFactory::loadConfigFile(const std::string
&fileName, std::strings
throw std::invalid_argument("Unable to load included file '" +
f + "' from '" + fileName + "': " + ex.what());
}
}
+ if (used_legacy_path) {
+ const auto recommended_include =
std::filesystem::path(f).lexically_relative(RecConfigReadConfigDir()).string();
+ NH_Warn("Deprecated '#include %s'; use '#include %s' instead.",
include_target.c_str(), recommended_include.c_str());
Review Comment:
`recommended_include` can be surprising/empty in edge cases (e.g., if
`lexically_relative` cannot compute a meaningful relative path). Consider
guarding the warning so it never suggests an empty string (fallback to a
known-safe suggestion like the basename, or skip the \"use ... instead\"
portion when the relative path isn’t usable).
--
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]
