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

images --format compatible with docker #5115

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

QiWang19
Copy link
Contributor

@QiWang19 QiWang19 commented Feb 6, 2020

Let valid values of --format be compatible with docker. Replace CreatedTime with CreatedAt, Created with CreatedSince.
fix #5110

Signed-off-by: Qi Wang [email protected]

@mheon
Copy link
Member

mheon commented Feb 6, 2020

LGTM but would prefer to hold until after 1.8.0 so I don't need to update release notes again

@rhatdan
Copy link
Member

rhatdan commented Feb 6, 2020

Any man page changes? Tests?
Also can I still use the old values? If I built a script using Created or CreatedTime, will it still work?

@QiWang19
Copy link
Contributor Author

QiWang19 commented Feb 6, 2020

Any man page changes? Tests?
Also can I still use the old values? If I built a script using Created or CreatedTime, will it still work?

We can add man pages like this https://docs.docker.com/engine/reference/commandline/images/#format-the-output
old value --format {{.Created}} won't work. Should be {{.CreatedSince}} now

@TomSweeneyRedHat
Copy link
Member

Looks like the same test errors here as in some other PRs. Houston we may have a CI problem.

CreatedSince string
Size string
ReadOnly bool
History string
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if we should still support "Created" and "CreatedTime" in case people have adjusted and have built dependencies on those? I'd be fine if they were hidden options going forward, available for use, but not documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern is that keeping Created and CreatedTime in this struct for hidden options seems duplicate.

Copy link
Member

Choose a reason for hiding this comment

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

yep, it's certainly a duplicate and I'd add a comment in the code as to why it's there. My concern is someone out there has already built a dependency on one of those and we might break them if they go away.

Copy link
Member

Choose a reason for hiding this comment

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

I would not retain duplicates in the structures - especially given how similar all these names are. We may want to look into normalization similar to what we're doing in podman inspect --format though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomSweeneyRedHat @mheon Thanks. I will do that.

Copy link
Member

Choose a reason for hiding this comment

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

If Matt's hip, then I'm hip. Thanks for the discussion!

@TomSweeneyRedHat
Copy link
Member

definitely double check on the man pages and tests. If we do drop the old names, we will need a BIG announcement in the 1.8.1 or 1.9.0 Release announcement about that.

@rhatdan
Copy link
Member

rhatdan commented Feb 7, 2020

LGTM
But not sure what is happening with the remote tests.

This patch lets valid values of --format be compatible with docker. Replace CreatedTime with CreatedAt, Created with CreatedSince.
Keep CreatedTime and Created are valid as hidden options.

Signed-off-by: Qi Wang <[email protected]>
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.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2020
@rhatdan
Copy link
Member

rhatdan commented Feb 13, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: QiWang19, 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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2020
@rhatdan
Copy link
Member

rhatdan commented Feb 13, 2020

/lgtm
/retry

@openshift-merge-robot openshift-merge-robot merged commit 2814995 into containers:master Feb 13, 2020
@QiWang19 QiWang19 deleted the images-format branch June 26, 2020 15:12
@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 Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman images --format incompatibility with docker
7 participants