szaszm commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4156779503
##########
.github/references/ubuntu_22_04_clang_arm_manifest.json:
##########
@@ -444,7 +444,7 @@
},
"Access Key": {
"name": "Access Key",
- "description": "AWS account access key",
+ "description": "AWS account access key.
DEPRECATED, please use AWS Credentials Provider service instead.",
Review Comment:
I'd put DEPRECATED as the first word of the description.
##########
libminifi/test/libtest/unit/TestUtils.h:
##########
@@ -69,6 +71,41 @@ std::string getFileContent(const std::filesystem::path&
file_name);
void makeFileOrDirectoryNotWritable(const std::filesystem::path& file_name);
void makeFileOrDirectoryWritable(const std::filesystem::path& file_name);
+/**
+ * Sets an environment variable for the lifetime of the object, and restores
its previous value (or unsets it, if it was
+ * not set before) on destruction. Pass std::nullopt as the value to make sure
the variable is not set in the scope.
+ * Environment variables are process-global, so tests that rely on them need
to clean up after themselves.
Review Comment:
I don't agree with the second part and the implication. Envvars are
process-global, but tests could also just leave them dirty, and let the process
eventually die.
##########
core-framework/common/src/utils/Environment.cpp:
##########
@@ -107,7 +107,13 @@ bool Environment::unsetEnvironmentVariable(const char*
name) {
Environment::accessEnvironment([&success, name](){
#ifdef WIN32
- success = SetEnvironmentVariableA(name, nullptr);
+ const bool crt_success = _putenv_s(name, "") == 0;
+ bool windows_success = SetEnvironmentVariableA(name, nullptr) != 0;
+ if (!windows_success && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
+ windows_success = true;
+ }
+
+ success = crt_success && windows_success;
Review Comment:
why do we need to do two ways of setting the envvar? Can you explain what
this fixes?
##########
libminifi/test/unit/EnvironmentUtilsTests.cpp:
##########
@@ -89,6 +90,16 @@ TEST_CASE("unsetenv existing", "[unsetenv]") {
REQUIRE(!utils::Environment::getEnvironmentVariable("UNSETENV2"));
}
+TEST_CASE("setenv and unsetenv are visible to getenv", "[setenv][unsetenv]") {
+ REQUIRE(true == utils::Environment::setEnvironmentVariable("GETENVSYNC",
"test"));
+ const char* const value = std::getenv("GETENVSYNC");
+ REQUIRE(value != nullptr);
+ CHECK(std::string{value} == "test");
+
+ REQUIRE(true == utils::Environment::unsetEnvironmentVariable("GETENVSYNC"));
+ CHECK(std::getenv("GETENVSYNC") == nullptr);
+}
+
Review Comment:
setenv is not thread-safe, so this test can only be run in single-threaded
processes.
--
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]
