ppkarwasz commented on code in PR #1323:
URL: https://github.com/apache/commons-lang/pull/1323#discussion_r1864810386
##########
src/main/java/org/apache/commons/lang3/RuntimeEnvironment.java:
##########
@@ -29,67 +31,59 @@
*/
public class RuntimeEnvironment {
+ // package-private non-static fields for testing.
+ static String rootDir = "/";
+
/**
- * Tests whether the file at the given path string contains a specific
line.
+ * Tests whether the /proc/N/environ file at the given path string
contains a specific line prefix.
*
- * @param path The path to a file.
- * @param line The line to find.
- * @return whether the file at the given path string contains a specific
line.
+ * @param envVarFile The path to a /proc/N/environ file.
+ * @param prefix The line prefix to find.
+ * @return value after the prefix
*/
- private static Boolean containsLine(final String path, final String line) {
- try (Stream<String> stream = Files.lines(Paths.get(path))) {
- return stream.anyMatch(test -> test.contains(line));
+ private static String getenv(final String envVarFile, final String prefix)
{
+ try {
+ byte[] bytes = Files.readAllBytes(Paths.get(envVarFile));
+ String content = new String(bytes, UTF_8);
+ // Split by null byte character
+ String[] lines = content.split("\u0000");
+ return Arrays.stream(lines).filter(test -> test.startsWith(prefix))
+ .map(test -> StringUtils.substringAfter(test, prefix))
+ .findFirst()
+ .orElse(null);
Review Comment:
_Nit_: Maybe we could split the lines using `line.split("=", 2)` first, so
we don't need to use `StringUtils` after.
##########
src/main/java/org/apache/commons/lang3/RuntimeEnvironment.java:
##########
@@ -29,67 +31,59 @@
*/
public class RuntimeEnvironment {
+ // package-private non-static fields for testing.
+ static String rootDir = "/";
+
/**
- * Tests whether the file at the given path string contains a specific
line.
+ * Tests whether the /proc/N/environ file at the given path string
contains a specific line prefix.
*
- * @param path The path to a file.
- * @param line The line to find.
- * @return whether the file at the given path string contains a specific
line.
+ * @param envVarFile The path to a /proc/N/environ file.
+ * @param prefix The line prefix to find.
+ * @return value after the prefix
*/
- private static Boolean containsLine(final String path, final String line) {
- try (Stream<String> stream = Files.lines(Paths.get(path))) {
- return stream.anyMatch(test -> test.contains(line));
+ private static String getenv(final String envVarFile, final String prefix)
{
+ try {
+ byte[] bytes = Files.readAllBytes(Paths.get(envVarFile));
+ String content = new String(bytes, UTF_8);
Review Comment:
The environment variables are not necessarily encoded in UTF-8. Looking at
the implementation of `System.getenv()`, the default character set seems to be
used.
##########
src/main/java/org/apache/commons/lang3/RuntimeEnvironment.java:
##########
@@ -29,67 +31,59 @@
*/
public class RuntimeEnvironment {
+ // package-private non-static fields for testing.
+ static String rootDir = "/";
Review Comment:
_Nit_: for testing I would prefer to create a package-private
`inContainer(String rootDir)` method, instead of a static field.
##########
src/test/java/org/apache/commons/lang3/RuntimeEnvironmentTest.java:
##########
@@ -17,21 +17,85 @@
package org.apache.commons.lang3;
-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.DynamicTest;
+import org.junit.jupiter.api.TestFactory;
+import org.junit.jupiter.api.io.TempDir;
-import org.junit.jupiter.api.Test;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.DynamicTest.dynamicTest;
/**
* Tests {@link RuntimeEnvironment}.
*/
public class RuntimeEnvironmentTest {
- @Test
- public void testIsContainer() {
- // At least make sure it does not blow up.
- assertDoesNotThrow(RuntimeEnvironment::inContainer);
- assertDoesNotThrow(RuntimeEnvironment::inDocker);
- assertDoesNotThrow(RuntimeEnvironment::inPodman);
- assertDoesNotThrow(RuntimeEnvironment::inWsl);
+ private static final String simpleEnviron =
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\u0000" +
+ "HOSTNAME=d62718b69f37\u0000TERM=xterm\u0000HOME=/root\u0000";
+
+ private static final String podmanEnviron =
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\u0000" +
+
"HOSTNAME=d62718b69f37\u0000TERM=xterm\u0000container=podman\u0000HOME=/root\u0000";
+
+ @TempDir
+ private Path tempDir;
+
+ @AfterAll
+ public static void tearDown() {
+ RuntimeEnvironment.rootDir = "/";
+ }
+
+ @TestFactory
+ public DynamicTest[] testIsContainerDocker() {
Review Comment:
Can you add a test case, where the `container` variable exists, but is empty?
Also: since all the test cases are pretty similar, could you use
`@ParameterizedTest` instead of `DynamicTest`?
##########
src/main/java/org/apache/commons/lang3/RuntimeEnvironment.java:
##########
@@ -29,67 +31,59 @@
*/
public class RuntimeEnvironment {
+ // package-private non-static fields for testing.
+ static String rootDir = "/";
+
/**
- * Tests whether the file at the given path string contains a specific
line.
+ * Tests whether the /proc/N/environ file at the given path string
contains a specific line prefix.
*
- * @param path The path to a file.
- * @param line The line to find.
- * @return whether the file at the given path string contains a specific
line.
+ * @param envVarFile The path to a /proc/N/environ file.
+ * @param prefix The line prefix to find.
+ * @return value after the prefix
*/
- private static Boolean containsLine(final String path, final String line) {
- try (Stream<String> stream = Files.lines(Paths.get(path))) {
- return stream.anyMatch(test -> test.contains(line));
+ private static String getenv(final String envVarFile, final String prefix)
{
+ try {
+ byte[] bytes = Files.readAllBytes(Paths.get(envVarFile));
+ String content = new String(bytes, UTF_8);
+ // Split by null byte character
+ String[] lines = content.split("\u0000");
+ return Arrays.stream(lines).filter(test -> test.startsWith(prefix))
+ .map(test -> StringUtils.substringAfter(test, prefix))
+ .findFirst()
+ .orElse(null);
} catch (final IOException e) {
- return false;
+ return null;
}
}
/**
* Tests whether we are running in a container like Docker or Podman.
*
- * @return whether we are running in a container like Docker or Podman.
+ * @return whether we are running in a container like Docker or Podman.
Never null
*/
public static Boolean inContainer() {
- return inDocker() || inPodman();
- }
+ /*
+ Roughly follow the logic in SystemD:
+
https://github.com/systemd/systemd/blob/0747e3b60eb4496ee122066c844210ce818d76d9/src/basic/virt.c#L692
- /**
- * Tests whether we are running in a Docker container.
- * <p>
- * Package-private for testing.
- * </p>
- *
- * @return whether we are running in a Docker container.
- */
- // Could be public at a later time.
- static Boolean inDocker() {
- return containsLine("/proc/1/cgroup", "/docker");
- }
+ We check the `container` environment variable of process 1:
+ If the variable is empty, we return false. This includes the case,
where the container developer wants to hide the fact that the application runs
in a container.
+ If the variable is not empty, we return true.
+ If the variable is absent, we continue.
- /**
- * Tests whether we are running in a Podman container.
- * <p>
- * Package-private for testing.
- * </p>
- *
- * @return whether we are running in a Podman container.
- */
- // Could be public at a later time.
- static Boolean inPodman() {
- return containsLine("/proc/1/environ", "container=podman");
+ We check files in the container. According to SystemD:
+ /.dockerenv is used by Docker.
+ /run/.containerenv is used by PodMan.
+
+ */
+ String value = getenv(rootDir + "proc/1/environ", "container=");
+ return StringUtils.isNotEmpty(value)
+ || fileExists(rootDir + ".dockerenv")
+ || fileExists(rootDir + "run/.containerenv");
Review Comment:
```suggestion
return value != null ?
!value.isEmpty() :
fileExists(rootDir + ".dockerenv") || fileExists(rootDir +
"run/.containerenv");
```
If `value` is not `null`, the files should not be checked.
--
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]
