Copilot commented on code in PR #1993:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1993#discussion_r2223574850
##########
minifi_main/MainHelper.cpp:
##########
@@ -111,3 +112,45 @@ std::filesystem::path determineMinifiHome(const
std::shared_ptr<logging::Logger>
return minifi_home;
}
+
+Locations getFromMinifiHome(const std::filesystem::path& minifi_home) {
+ return {
+ .working_dir_ = minifi_home,
+ .lock_path_ = minifi_home / "LOCK",
+ .log_properties_path_ = minifi_home / DEFAULT_LOG_PROPERTIES_FILE,
+ .uid_properties_path_ = minifi_home / DEFAULT_UID_PROPERTIES_FILE,
+ .properties_path_ = minifi_home / DEFAULT_NIFI_PROPERTIES_FILE,
+ .logs_dir_ = minifi_home / "logs",
+ .fips_bin_path_ = minifi_home / "fips",
+ .fips_conf_path_ = minifi_home / "fips",
+ };
+}
+
+Locations getFromFHS() {
+ return {
+ .working_dir_ = std::filesystem::path(RPM_WORK_DIR),
+ .lock_path_ = std::filesystem::path(RPM_WORK_DIR) / "LOCK",
+ .log_properties_path_ = std::filesystem::path(RPM_CONFIG_DIR) /
"minifi-log.properties",
+ .uid_properties_path_ = std::filesystem::path(RPM_CONFIG_DIR) /
"minifi-uid.properties",
+ .properties_path_ = std::filesystem::path(RPM_CONFIG_DIR) /
"minifi.properties",
+ .logs_dir_ = std::filesystem::path(RPM_LOG_DIR),
+ .fips_bin_path_ = std::filesystem::path(RPM_LIB_DIR) / "fips",
+ .fips_conf_path_ = std::filesystem::path(RPM_CONFIG_DIR) / "fips"
+ };
+}
+
+std::optional<Locations> determineLocations(const
std::shared_ptr<logging::Logger>& logger) {
+ if (const auto minifi_home_env =
utils::Environment::getEnvironmentVariable(MINIFI_HOME_ENV_KEY)) {
+ if (minifi_home_env == "FHS") {
Review Comment:
The logic checks for MINIFI_HOME="FHS" as a special case, but this magic
string should be documented or defined as a constant to make the behavior
clearer and prevent typos.
```suggestion
if (minifi_home_env == FHS_ENV_VALUE) {
```
##########
minifi_main/Fips.cpp:
##########
@@ -94,37 +94,42 @@ bool generateFipsModuleConfig(const std::filesystem::path&
minifi_home, const st
}
} // namespace
-void initializeFipsMode(const std::shared_ptr<minifi::Configure>& configure,
const std::filesystem::path& minifi_home, const
std::shared_ptr<core::logging::Logger>& logger) {
+void initializeFipsMode(const std::shared_ptr<minifi::Configure>& configure,
const Locations& locations, const std::shared_ptr<core::logging::Logger>&
logger) {
+ const auto& fips_bin_path = locations.fips_bin_path_;
+ const auto& fips_conf_path = locations.fips_conf_path_;
if (!(configure->get(minifi::Configure::nifi_openssl_fips_support_enable) |
utils::andThen(utils::string::toBool)).value_or(false)) {
logger->log_info("FIPS mode is disabled. FIPS configs and modules will NOT
be loaded.");
return;
}
- if (!std::filesystem::exists(minifi_home / "fips" / FIPS_LIB)) {
- logger->log_error("FIPS mode is enabled, but {} is not available in
MINIFI_HOME/fips directory", FIPS_LIB);
+ if (!std::filesystem::exists(fips_bin_path / FIPS_LIB)) {
+ logger->log_error("FIPS mode is enabled, but {} is not available in {}
directory", FIPS_LIB, fips_bin_path);
std::exit(1);
}
- if (!std::filesystem::exists(minifi_home / "fips" / "fipsmodule.cnf") &&
!generateFipsModuleConfig(minifi_home, logger)) {
- logger->log_error("FIPS mode is enabled, but fipsmodule.cnf is not
available in $MINIFI_HOME/fips directory, and minifi couldn't generate it
automatically. "
- "Run $MINIFI_HOME/fips/openssl fipsinstall -out fipsmodule.cnf -module
$MINIFI_HOME/fips/{} command to generate the configuration file", FIPS_LIB);
+ if (!std::filesystem::exists(fips_conf_path / "fipsmodule.cnf") &&
!generateFipsModuleConfig(locations, logger)) {
+ logger->log_error("FIPS mode is enabled, but fipsmodule.cnf is not
available in {fips_conf_dir} directory, and minifi couldn't generate it
automatically. "
+ "Run {fips_bin_dir}/openssl fipsinstall -out
{fips_conf_dir}/fipsmodule.cnf -module {fips_bin_dir}/{fips_lib_name} command
to generate the configuration file",
+ fmt::arg("fips_conf_dir", fips_conf_path),
+ fmt::arg("fips_bin_dir", fips_bin_path),
+ fmt::arg("fips_lib_name", FIPS_LIB));
Review Comment:
[nitpick] The log message spans multiple lines and uses fmt::arg for
formatting, but the message construction is complex and could be simplified for
better readability.
```suggestion
logger->log_error(fmt::format("FIPS mode is enabled, but fipsmodule.cnf
is not available in {} directory, and minifi couldn't generate it
automatically. "
"Run {}/openssl fipsinstall -out {}/fipsmodule.cnf -module {}/{}
command to generate the configuration file",
fips_conf_path, fips_bin_path, fips_conf_path, fips_bin_path,
FIPS_LIB));
```
##########
generateVersion.sh:
##########
@@ -71,7 +71,7 @@ const char* const AgentBuild::BUILD_REV = "$buildrev";
const char* const AgentBuild::BUILD_DATE = "$date";
const char* const AgentBuild::COMPILER = "$compiler";
const char* const AgentBuild::COMPILER_VERSION = "$compiler_version";
-const char* const AgentBuild::COMPILER_FLAGS = "$flags";
+const char* const AgentBuild::COMPILER_FLAGS = R"($flags)";
Review Comment:
Using raw string literals for compiler flags is good, but this change should
be accompanied by a corresponding update in generateVersion.bat to ensure
consistency across platforms.
##########
extensions/python/pythonprocessors/nifi_python_processors/utils/dependency_installer.py:
##########
@@ -64,3 +64,4 @@ def extract_dependencies(file_path):
if dependencies_found:
subprocess.check_call(command)
+ print("Done installing dependencies for MiNiFi python processors.")
Review Comment:
This print statement appears to be debugging code that should be removed or
converted to proper logging. Raw print statements in production code can
interfere with structured logging and output parsing.
##########
docker/test/integration/cluster/DockerTestCluster.py:
##########
@@ -329,7 +332,7 @@ def check_minifi_log_does_not_contain(self, line,
wait_time_seconds):
def wait_for_container_startup_to_finish(self, container_name):
container_name =
self.container_store.get_container_name_with_postfix(container_name)
- startup_success = self.wait_for_startup_log(container_name, 300)
+ startup_success = self.wait_for_startup_log(container_name, 160)
Review Comment:
The startup timeout was increased from 300 to 160 seconds, which is actually
a decrease. This might cause startup failures for slower systems or complex
configurations.
```suggestion
startup_success = self.wait_for_startup_log(container_name, 300)
```
--
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]
