-
Notifications
You must be signed in to change notification settings - Fork 0
top: add support for --layers #8
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
base: main
Are you sure you want to change the base?
Conversation
4fe3b04 to
c9d0e86
Compare
c9d0e86 to
700e26f
Compare
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 overall, just added a few minor points
700e26f to
fafe6a6
Compare
|
@danishprakash rebase pls 🥺 |
fba79fc to
bd617c2
Compare
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.
Overall very good, I have added a bunch of nitpicks and suggestions all over the place.
I do have one nomenclature fixing request: we must really be consistent with naming here. The new top feature calls image digests sometimes digests and sometimes IDs, which is not consistent with podman's naming. Iirc the layer ID is some internal representation, while the digest is defined in the image. Hence I would suggest, that we use the name digest everywhere instead of ID.
2780dcf to
6ee6b17
Compare
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 like where you are going with the ID changes to use the image layer digest, but the naming is still inconsistent and now the UX is more confusing, because you filter by layer digest and display the registry digest.
e323b95 to
4e9e89c
Compare
|
|
||
| // getLayersByDiffID returns layer blob infos filtered by user-provided diffIDs | ||
| // by looking up diffIDs and mapping to manifest layers | ||
| func getLayersByDiffID(manifestLayers []types.BlobInfo, allDiffIDs []digest.Digest, filterDiffIDs []string) ([]types.BlobInfo, []digest.Digest, error) { |
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.
Isn't BlobInfo.Digest usable in this context?
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.
What do you mean?
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.
the manifestLayers array is of type types.BlobInfo, which has a field Digest. Why not use that Digest instead of passing the additional allDiffIDs array with the digests as a separate parameter?
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.
because manifestLayers[x].Digest isn't guaranteed to be the equivalent of diffID, it could also represent a compressed digest if the image is not in the local storage, for instance.
Signed-off-by: Danish Prakash <[email protected]>
Get top files for one or more layers specified via digest, partial or complete. Errors out if the passed layers don't match any layer in the image, or it matches more than one layers. Merges the output when more than one layer is passed. * add unit tests Signed-off-by: Danish Prakash <[email protected]>
Signed-off-by: Danish Prakash <[email protected]>
Signed-off-by: Danish Prakash <[email protected]>
Co-authored-by: Dan Čermák <[email protected]>
bfbfdf4 to
fd60d83
Compare
7b9bfa8 to
dcfdf4c
Compare
Signed-off-by: Danish Prakash <[email protected]>
dcfdf4c to
c72d46a
Compare
Co-authored-by: Dan Čermák <[email protected]>
Co-authored-by: Dan Čermák <[email protected]>
Signed-off-by: Danish Prakash [email protected]