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

Fixes format inconsistencies with docker for certain history fields #18213

Merged
merged 2 commits into from Apr 20, 2023

Conversation

rbagd
Copy link
Contributor

@rbagd rbagd commented Apr 15, 2023

Closes #17767
Closes #17768

Does this PR introduce a user-facing change?

Fixes a format inconsistency where podman and docker return slightly different output for `history --format {{.CreatedAt}}` and `history --format {{.Size}}` (#17767, #17768)

@rbagd
Copy link
Contributor Author

rbagd commented Apr 15, 2023

CI indicates that one of the bats tests failed, namely podman image history Created. Besides a regex check which is fixable, it also requires that podman image list --format {{.CreatedAt}} would be the same as podman image history --format {{.CreatedAt}}. But Docker itself outputs them in a different format, so it's unclear now if this comparison should be dropped to favor docker compatibility... 🤷‍♂️

Assuming PR is merged,

$ docker image list --format {{.CreatedAt}} busybox | head -n 1
2023-03-17 02:35:43 +0100 CET
$ docker image history --format {{.CreatedAt}} busybox | head -n 1
2023-03-17T02:35:43+01:00

$ podman image list --format {{.CreatedAt}} busybox | head -n 1
2023-03-17 01:35:43 +0000 UTC
$ podman image history --format {{.CreatedAt}} busybox | head -n 1
2023-03-17T02:35:43+01:00

@rhatdan
Copy link
Member

rhatdan commented Apr 16, 2023

I am fine with fixing this, but I will allow @vrothberg and @mheon to chime in.

@mheon
Copy link
Member

mheon commented Apr 17, 2023

I'd prefer to retain the test - it seems like it could be changed to parse the fields back to a time.Time and compare them that way. LGTM otherwise.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

One system test isn't passing (see https://cirrus-ci.com/task/5421900956958720?logs=main#L284). @edsantiago, we probably need to your scripting skills. I tried normalizing the two dates but one fails with

$ date -d '2022-10-18 16:28:08 +0000 UTC'

test/e2e/history_test.go Outdated Show resolved Hide resolved
@edsantiago
Copy link
Collaborator

The failing system test is: CreatedSince and CreatedAt from image history should == image list. TL;DR this PR makes podman image list and podman image history inconsistent with each other:

$ for i in list history;do echo;bin/podman image $i --format '{{.CreatedAt}}' quay.io/libpod/testimage:20221018;done

2022-10-18 16:28:08 +0000 UTC     <--- until now, this has been the format for both

2022-10-18T10:28:08-06:00         <--- new, changed format
2022-10-18T10:28:08-06:00
2022-10-18T10:28:08-06:00
2022-10-18T10:28:08-06:00
2022-10-18T10:28:20-06:00
2022-10-18T10:28:20-06:00
2022-10-18T10:28:20-06:00

@rbagd rbagd force-pushed the main branch 3 times, most recently from 8718afb to d2c01fe Compare April 17, 2023 18:54
@eriksjolund
Copy link
Contributor

eriksjolund commented Apr 18, 2023

In the commit message it's written

Closes #17767 and #17768 

The GitHub linking only works for the first one (17767) but not the second one (17768)

I would guess the commit message needs to be written like this for it to work

Closes #17767
Closes #17768 

Quote:

Multiple issues	Use full syntax for each issue

from
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

@rbagd
Copy link
Contributor Author

rbagd commented Apr 18, 2023

I updated the relevant system test to maintain the equality check of podman image history and podman image list output for CreatedAt and CreatedSince. Please let me know if this looks good enough. To make the whole thing more readable I switched field separator in the test to ; from --.

Closes containers#17767
Closes containers#17768

System test for image list and history dates

* Changed field separator in the test to `;` for easier parsing
* Converted date output from image history and image list to be comparable

Signed-off-by: rbagd <mail@rbagd.eu>
@Luap99
Copy link
Member

Luap99 commented Apr 19, 2023

@edsantiago PTAL at system tests

Following @edsantiago guidance,

* Additional explanations for each step of the test
* Timezone for tests normalized to UTC
* Smarter choice of separator and use of shell substring extraction

Signed-off-by: rbagd <mail@rbagd.eu>
@edsantiago
Copy link
Collaborator

LGTM. Thanks @rbagd.

@containers/podman-maintainers PTAL, especially the docker compatibility (I don't have a good way to test docker).

@eriksjolund
Copy link
Contributor

@rbagd Thanks for adjusting the commit message so that it now says

Closes #17767
Closes #17768

Unfortunately it didn't fix the the GitHub linking. Sorry for my misleading information.
I made a test in another GitHub repository and saw that we can fix the GitHub linking by editing the pull request's description and replace

Closes #17767 and #17768

with

Closes #17767
Closes #17768

@rbagd
Copy link
Contributor Author

rbagd commented Apr 20, 2023

It seems to be working! Both issues appear on the right sidebar now as expected.

@TomSweeneyRedHat
Copy link
Member

LGTM

@rhatdan
Copy link
Member

rhatdan commented Apr 20, 2023

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rbagd, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2023
@openshift-ci openshift-ci bot merged commit 69ec2d6 into containers:main Apr 20, 2023
80 checks passed
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
8 participants