docker-workflow-plugin
docker-workflow-plugin copied to clipboard
jenkinsci
JENKINS-57589 Fix parsing of container creation date
cc @jglick who was involved in the related PR https://github.com/jenkinsci/docker-workflow-plugin/pull/15: PTAL
@dwnusbaum thanks for volunteering to review this Devin - could you check it over when you get a minute please? The changes are pretty straightforward.
The fix seems simple/fine, but I don't really understand the impact (is the creation date even used for anything now that fingerprinting is disabled by default? Was it ever used?).
IIRC the problem surfaced because I was using this plugin with podman (rather than Docker) and the timestamps it exposes via podman inspect, although ISO 8601 compliant, don't conform to the more restrictive assumptions made by this plugin's old code, causing it to throw an exception.
I believe (can't easily check, sorry) that Docker outputs timestamps pre-converted to UTC, whereas podman outputs them in the system's TZ. The old code assumed that removing the final character from the timestamp would be harmless (because in UTC this is always just Z), so it choked on non-UTC timestamps which end with e.g. -0700.
It looks like the PR could still use a regression test that deals with time zones specifically (unless the changes to DockerClientTest fail without the fix?).
Unfortunately because Docker always returns timestamps in UTC, you can't really test for the actual bug unless you start running your tests against podman (in a non-UTC tz) as well. :man_shrugging:
I tightened up the tests while I was here, but you're correct that they won't actually catch a regression.