Podman history now prints out intermediate image IDs #982
Conversation
|
@rhatdan @mheon @edsantiago PTAL |
|
Example: |
cmd/podman/history.go
Outdated
| imageID = imageIDs[imgIDCount] | ||
| imgIDCount++ | ||
| } else { | ||
| imageID = "<none>" |
There was a problem hiding this comment.
For the sake of docker consistency, should this (and related changes) remain as <missing>?
There was a problem hiding this comment.
Yup, fixed. Not sure why I thought it was none...
libpod/image/image.go
Outdated
| @@ -609,7 +609,7 @@ func (i *Image) Layer() (*storage.Layer, error) { | |||
| } | |||
|
|
|||
| // History gets the history of an image and information about its layers | |||
| func (i *Image) History(ctx context.Context) ([]ociv1.History, []types.BlobInfo, error) { | |||
| func (i *Image) History(ctx context.Context, imageIDs *[]string) ([]ociv1.History, []types.BlobInfo, error) { | |||
There was a problem hiding this comment.
Why are we passing in imageIDs here? Can't we compute this from the image's own ID?
There was a problem hiding this comment.
It seems like we should be returning imageIDs instead. Also, we should probably just return an array of one struct, containing the ociv1.History, types.BlobInfo, and image ID for the layer in history, so we don't have to worry about potentially mismatched array sizes.
There was a problem hiding this comment.
I want to pass in an array that I can modify, so that is why I was doing that. But you are right I can just return it instead. I am taking care of the size mismatch in cmd/podman/history.go.
Also we need to consider the order ocivi.History and types.BlobInfo are returned in. I think they are opposite of each other and this is also taken care of in cmd/podman/history.go.
Changing it one struct here will add more complexity, but if that is preferred I can make the change.
There was a problem hiding this comment.
I think that returning one struct is probably better than returning sets of arrays that are potentially in reverse order... That seems really not-obvious to work with.
As a bonus, it might let us remove code duplication with varlink, which is always good.
Sorry for the extra work!
There was a problem hiding this comment.
sure, no worries! Working on it.
libpod/image/image.go
Outdated
| return nil, nil, errors.Wrapf(err, "error getting images from store") | ||
| } | ||
| if err := i.historyLayerIDs(i.TopLayer(), images, imageIDs); err != nil { | ||
| return nil, nil, errors.Wrap(err, "error getting image IDs for layers in history") |
There was a problem hiding this comment.
nit two spaces before 'history'
umohnani8
force-pushed
the
history
branch
2 times, most recently
from
f71ccd8 to
ef5cd83
Compare
|
@mheon reworked it, PTAL |
libpod/image/image.go
Outdated
| // History gets the history of an image and information about its layers | ||
| func (i *Image) History(ctx context.Context) ([]ociv1.History, []types.BlobInfo, error) { | ||
| func (i *Image) History(ctx context.Context) ([]History, error) { |
There was a problem hiding this comment.
Nit: can we return []*History instead, to avoid a copy?
|
LGTM. Thanks @umohnani8 - looks a lot cleaner now. |
|
📌 Commit ef5cd83 has been approved by |
|
LGTM, especially if you can convert to a pointer per @mheon. |
|
@umohnani8 I think if you push another commit it should cancel the merge, so you should be fine. And the good news is that there are a good 4 or 5 PRs in line ahead of this one, so you have plenty of time |
|
@umohnani8 I would not change this PR unless it doesn't merge. I'd do a second PR after this one gets in. |
|
and always listen to @mheon over lucky-sweeney. |
If the intermediate image exists in the store, podman history will show the IDs of the intermediate image of each layer. Signed-off-by: umohnani8 <[email protected]>
|
@mheon should be fixed now. Thanks for the tip :) |
|
📌 Commit 18ebbfa has been approved by |
|
☀️ Test successful - status-papr |
github-actions
bot
added
the
locked - please file new issue/PR
If the intermediate image exists in the store, podman history
will show the IDs of the intermediate image of each layer.
Fixes #980
Signed-off-by: umohnani8 [email protected]