Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for docker timestamps with timezone offsets #4073

Merged
merged 2 commits into from
Jul 16, 2021

Conversation

candrews
Copy link
Contributor

Docker prefers to use UTC time when making images/containers, but it's happy to parse non-UTC times too. It will then pass those on in its output.

I originally discovered this issue while testing with podman, but it also applies to Docker. For example, from containers/podman#10267 (comment) :

Docker prefers to use UTC time when making images/containers, but it's happy to parse non-UTC times too. It will then pass those on in its output. For example:

C:\Users\***>docker images
REPOSITORY                             TAG                 IMAGE ID            CREATED             SIZE
mcr.microsoft.com/windows/nanoserver   10.0.14393.1066     a943c29f0046        4 years ago         1.01GB

C:\Users\***>docker image inspect a943c29f0046
[
    {
        "Id": "sha256:a943c29f0046aa2764d32bf9dfeca1d816865729e3fa26be0259915bf78ac05e",
        "RepoTags": [
            "mcr.microsoft.com/windows/nanoserver:10.0.14393.1066"
        ],
        "RepoDigests": [
            "mcr.microsoft.com/windows/nanoserver@sha256:ded482e81f381c94458a1d12d995506ac331d6d3180ed6da24ba809b0849a46c"
        ],
        "Parent": "",
        "Comment": "",
        "Created": "2017-04-11T02:12:36.3606843-07:00",
        [...]

@bsideup
Copy link
Member

bsideup commented May 10, 2021

@candrews nice, thanks for spotting this! Could you please add a (parameterized?) test to make sure that we won't introduce a regression in the future? Thanks!

@bsideup bsideup added this to the next milestone May 10, 2021
@candrews
Copy link
Contributor Author

Could you please add a (parameterized?) test to make sure that we won't introduce a regression in the future?

I don't think a parameterized test is necessary. DateTimeFormatter.ISO_OFFSET_DATE_TIME accepts everything DateTimeFormatter.ISO_INSTANT accepts and more, so replacing ISO_INSTANT with ISO_OFFSET_DATE_TIME in the existing test should prevent any future regressions.

@matejvasek
Copy link

@candrews does DateTimeFormatter.ISO_OFFSET_DATE_TIME.format(instant) work for you? I am getting exception with that.
java.time.temporal.UnsupportedTemporalTypeException: Unsupported field: Year.

@matejvasek
Copy link

@candrews I think that @bsideup point is that this doesn't test new extended functionality (accepting TS with TZ). So if somebody did some changes leading to not supporting TZ again it wouldn't be caught by tests.

@matejvasek
Copy link

matejvasek commented May 11, 2021

@candrews
sample solotion:

diff --git a/core/src/test/java/org/testcontainers/utility/DockerStatusTest.java b/core/src/test/java/org/testcontainers/utility/DockerStatusTest.java
index 4b63719f..abaa3cb3 100644
--- a/core/src/test/java/org/testcontainers/utility/DockerStatusTest.java
+++ b/core/src/test/java/org/testcontainers/utility/DockerStatusTest.java
@@ -2,11 +2,16 @@ package org.testcontainers.utility;
 
 import com.github.dockerjava.api.command.InspectContainerResponse;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 import org.mockito.Mockito;
 
 import java.time.Duration;
 import java.time.Instant;
+import java.time.ZoneOffset;
 import java.time.format.DateTimeFormatter;
+import java.util.TimeZone;
+import java.util.function.Function;
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -15,33 +20,53 @@ import static org.mockito.Mockito.when;
 /**
  *
  */
+@RunWith(Parameterized.class)
 public class DockerStatusTest {
 
     private static Instant now = Instant.now();
 
-    private static InspectContainerResponse.ContainerState running =
-        buildState(true, false, buildTimestamp(now.minusMillis(30)), DockerStatus.DOCKER_TIMESTAMP_ZERO);
-
-    private static InspectContainerResponse.ContainerState runningVariant =
-        buildState(true, false, buildTimestamp(now.minusMillis(30)), "");
-
-    private static InspectContainerResponse.ContainerState shortRunning =
-        buildState(true, false, buildTimestamp(now.minusMillis(10)), DockerStatus.DOCKER_TIMESTAMP_ZERO);
-
-    private static InspectContainerResponse.ContainerState created =
-        buildState(false, false, DockerStatus.DOCKER_TIMESTAMP_ZERO, DockerStatus.DOCKER_TIMESTAMP_ZERO);
-
-    // a container in the "created" state is not running, and has both startedAt and finishedAt empty.
-    private static InspectContainerResponse.ContainerState createdVariant =
-        buildState(false, false, null, null);
-
-    private static InspectContainerResponse.ContainerState exited =
-        buildState(false, false, buildTimestamp(now.minusMillis(100)), buildTimestamp(now.minusMillis(90)));
+    private static Duration minimumDuration = Duration.ofMillis(20);
 
-    private static InspectContainerResponse.ContainerState paused =
-        buildState(false, true, buildTimestamp(now.minusMillis(100)), DockerStatus.DOCKER_TIMESTAMP_ZERO);
+    @Parameterized.Parameter(0)
+    public InspectContainerResponse.ContainerState running;
+    @Parameterized.Parameter(1)
+    public InspectContainerResponse.ContainerState runningVariant;
+    @Parameterized.Parameter(2)
+    public InspectContainerResponse.ContainerState shortRunning;
+    @Parameterized.Parameter(3)
+    public InspectContainerResponse.ContainerState created;
+    @Parameterized.Parameter(4)
+    public InspectContainerResponse.ContainerState createdVariant;
+    @Parameterized.Parameter(5)
+    public InspectContainerResponse.ContainerState exited;
+    @Parameterized.Parameter(6)
+    public InspectContainerResponse.ContainerState paused;
+    @Parameterized.Parameter(7)
+    public String name;
+
+
+    private static Object[] createTestData(Function<Instant,String> formatter, String name) {
+        return new Object[] {
+            buildState(true, false, formatter.apply(now.minusMillis(30)), DockerStatus.DOCKER_TIMESTAMP_ZERO),
+            buildState(true, false, formatter.apply(now.minusMillis(30)), ""),
+            buildState(true, false, formatter.apply(now.minusMillis(10)), DockerStatus.DOCKER_TIMESTAMP_ZERO),
+            buildState(false, false, DockerStatus.DOCKER_TIMESTAMP_ZERO, DockerStatus.DOCKER_TIMESTAMP_ZERO),
+            // a container in the "created" state is not running, and has both startedAt and finishedAt empty.
+            buildState(false, false, null, null),
+            buildState(false, false, formatter.apply(now.minusMillis(100)), formatter.apply(now.minusMillis(90))),
+            buildState(false, true, formatter.apply(now.minusMillis(100)), DockerStatus.DOCKER_TIMESTAMP_ZERO),
+            name
+        };
+    }
 
-    private static Duration minimumDuration = Duration.ofMillis(20);
+    @Parameterized.Parameters(name = "{7}")
+    public static Object[][] data() {
+        return new Object[][] {
+            createTestData(DateTimeFormatter.ISO_INSTANT::format, "with UTC"),
+            createTestData(inst -> inst.atOffset(ZoneOffset.ofHours(1)).format(DateTimeFormatter.ISO_OFFSET_DATE_TIME), "with positive offset"),
+            createTestData(inst -> inst.atOffset(ZoneOffset.ofHoursMinutes(-1, -30)).format(DateTimeFormatter.ISO_OFFSET_DATE_TIME), "with negative offset"),
+        };
+    }
 
     @Test
     public void testRunning() throws Exception {
@@ -65,10 +90,6 @@ public class DockerStatusTest {
         assertFalse(DockerStatus.isContainerStopped(paused));
     }
 
-    private static String buildTimestamp(Instant instant) {
-        return DateTimeFormatter.ISO_INSTANT.format(instant);
-    }
-
     // ContainerState is a non-static inner class, with private member variables, in a different package.
     // It's simpler to mock it that to try to create one.
     private static InspectContainerResponse.ContainerState buildState(boolean running, boolean paused,

@candrews
Copy link
Contributor Author

Is this PR now mergeable?
If not, please let me know what I can do.

Thank you!

@bsideup
Copy link
Member

bsideup commented May 15, 2021

Hi @candrews,

Could you please rework the tests, so that there is only one ContainerState parameter? That's how usually parameterized tests are used. Thanks!

@candrews
Copy link
Contributor Author

Hi @candrews,

Could you please rework the tests, so that there is only one ContainerState parameter? That's how usually parameterized tests are used. Thanks!

I adjusted it so there aren't any ContainerState parameters - the DateTimeFormatter is now the parameter. I think that makes the test more readable and is more like the original test.

Docker prefers to use UTC time when making images/containers,
but it's happy to parse non-UTC times too. It will then pass those on in its
output.

DateTimeFormatter.ISO_OFFSET_DATE_TIME accepts a superset of what
DateTimeFormatter.ISO_INSTANT accepts, so replace use of
ISO_INSTANT with ISO_OFFSET_DATE_TIME.
@candrews
Copy link
Contributor Author

candrews commented Jun 3, 2021

I believe that I've addressed the feedback received so far. Can this please be reviewed again and (potentially) merged?

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me, thanks @candrews and sorry this has taken a long time to review.

FWIW, I think there's something quite interesting here: I was unable to replicate the original problem locally (and this glot.io snippet illustrates this, I think). This seems bizarre, so I did a bit of digging. Apparently this problem was actually considered a bug in the JDK which was fixed in openjdk/jdk@f5d19b9 (which seems to have made it into JDK12+).

Perhaps this explains why we've not heard about this problem more often.

But regardless, we're not about to drop support for older JDKs, so I'm very happy to accept this PR.

@rnorth rnorth merged commit e89dce1 into testcontainers:master Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants