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

JENKINS-57589 Fix parsing of container creation date #175

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rufoa
Copy link

@rufoa rufoa commented May 21, 2019

Thought I'd fix this outstanding TODO

@rufoa rufoa changed the title Fix parsing of container creation date JENKINS-57589 Fix parsing of container creation date May 21, 2019
@rufoa
Copy link
Author

rufoa commented May 21, 2019

JIRA issue here

@rufoa
Copy link
Author

rufoa commented May 22, 2019

cc @jglick who was involved in the related PR #15: PTAL

@rufoa
Copy link
Author

rufoa commented Aug 24, 2019

(3 months without response)

@dwnusbaum PTAL. Thanks

@dwnusbaum
Copy link
Member

dwnusbaum commented Aug 26, 2019

@rufoa Thanks for the PR! Can you add a regression test for the bug?

@dwnusbaum dwnusbaum self-requested a review August 26, 2019 21:15
@rufoa
Copy link
Author

rufoa commented Aug 26, 2019

All done I think

@rufoa
Copy link
Author

rufoa commented Nov 11, 2019

@dwnusbaum thanks for volunteering to review this Devin - could you check it over when you get a minute please? The changes are pretty straightforward.

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

@rufoa 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?). 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?).

@rufoa
Copy link
Author

rufoa commented Nov 12, 2019

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. 🤷‍♂️

I tightened up the tests while I was here, but you're correct that they won't actually catch a regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants