-
Notifications
You must be signed in to change notification settings - Fork 460
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
Add show pinned information for images #1247
Add show pinned information for images #1247
Conversation
Signed-off-by: Iceber Gu <caiwei95@hotmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
Can we add a release note to the body of this PR to document the change?
@saschagrunert updated, ptal |
/approve |
There is a"typo" in the description and commits, it says Verified that it also works with Docker, presuming that Mirantis/cri-dockerd#194 is patched into
|
89b5d79
to
a248040
Compare
Sorry, I misspelled the name of the command. |
cmd/crictl/image.go
Outdated
showDigest := c.Bool("digests") | ||
quiet := c.Bool("quiet") | ||
noTrunc := c.Bool("no-trunc") | ||
if !verbose && !quiet { | ||
row := []string{columnImage, columnTag} | ||
if showPinned { | ||
row = append(row, columnPinned) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a new column to the end? The column is not that important and in most cases will only be true for a single image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking is that when the user passes the --pinned flag, maybe he is trying to view the pinned information, so I put this column in front of it.
if I put it at the end this is what it would look like, one more line of code:
row := []string{columnImage, columnTag}
if showPinned {
row = append(row, columnPinned)
}
row = append(row, columnImageID, columnSize)
if showDigest {
row = append(row, columnDigest)
}
display.AddRow(row)
Of course, I'm keeping an open mind about the placement of this column, if you suggest putting it at the end , then I'll update this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand the history here. I think the --digests
was added to match docker cli
. And docker has it because there were no digests for older images: https://docs.docker.com/engine/reference/commandline/images/#digests
If we want to keep compatibility with docker
, having an additional flag and not changing the default output makes sense.
It may be a good idea to create a test for this and explicitly mention that the ordering of columns and the default list of columns is driven by compatibility with the docker cli.
Ordering-wise I really don't like the idea of inserting column in the middle. If customer wants it - they can use template to specify the exact order of columns.
My suggestion will be to put pinned to be the last column.
@@ -155,6 +155,10 @@ var listImageCommand = &cli.Command{ | |||
Name: "no-trunc", | |||
Usage: "Show output without truncating the ID", | |||
}, | |||
&cli.BoolFlag{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why wouldn't we just output it by default without an explicit flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think this field was that important, so I left the default output format unchanged.
Of course I'm not particularly sure, I'll take everyone's advice on this
Signed-off-by: Iceber Gu <caiwei95@hotmail.com>
a248040
to
7d7fd9b
Compare
@SergeyKanzhelev I've updated the description and code, PTAL, Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Iceber, saschagrunert, SergeyKanzhelev 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add showing of pinned information when using the
crictl images
command.crictl images -v
prints Pinned when image is pinnedcrictl images
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?