Add size analyzer #256
Add size analyzer #256
Conversation
There was a problem hiding this comment.
@peter-evans thanks for the contribution! this is super cool. overall the structure looks good, I left a few comments and one change request.
differs/size_diff.go
Outdated
| func (a SizeAnalyzer) Diff(image1, image2 pkgutil.Image) (util.Result, error) { | ||
| diff := []util.EntryDiff{ | ||
| { | ||
| Name: "*", |
There was a problem hiding this comment.
is there a reason you used * here? should this just be the empty string?
There was a problem hiding this comment.
The reason there is an * here is because it indicates "all layers" of the image. I was trying to use templates efficiently and just created two for the four different cases.
The four cases are:
analyze <image> --type=sizeanalyze <image> --type=sizelayerdiff <image1> <image2> --type=sizediff <image1> <image2> --type=sizelayer
Rather than create four separate templates I found a way to create two, one for analysis and one for diff. Both templates have LAYER as the first column and I reuse them for the image size comparison by displaying an *. Here is an example:
$ out/container-diff analyze gcr.io/gcp-runtimes/diff-layer-base --type=size
-----Size-----
Analysis for gcr.io/gcp-runtimes/diff-layer-base:
LAYER SIZE
* 19BWhen analyzing/diffing the image layers the layer number is shown as in this example:
$ out/container-diff analyze gcr.io/gcp-runtimes/diff-layer-base --type=sizelayer
-----SizeLayer-----
Analysis for gcr.io/gcp-runtimes/diff-layer-base:
LAYER SIZE
0 6B
1 7B
2 6BIf you would prefer to have the column read IMAGE for the image size comparison cases I can create two further templates. Let me know what you think.
There was a problem hiding this comment.
Ah I see what you did. Being totally honest, I'm not a huge fan of reusing LAYER for the image diff, I think it's a little confusing. If you could create another template for this case that would be great.
| -----{{.DiffType}}----- | ||
|
|
||
| Size differences between {{.Image1}} and {{.Image2}}:{{if not .Diff}} None{{else}} | ||
| LAYER SIZE1 SIZE2{{range .Diff}}{{"\n"}}{{.Name}} {{.Size1}} {{.Size2}}{{end}} |
There was a problem hiding this comment.
should this be IMAGE instead of LAYER?
There was a problem hiding this comment.
Please see this comment: #256 (comment)
tests/size_layer_diff_expected.json
Outdated
| { | ||
| "Name": "0", | ||
| "Size1": 6, | ||
| "Size2": 6 |
There was a problem hiding this comment.
hmm. any idea why this is showing up when the two sizes are the same?
There was a problem hiding this comment.
The way I've implemented this is technically not a "diff." In both cases, image and layer size diff, it just shows a comparison between the two even if they match. Now that you've pointed it out I've realised that the diff output probably needs a rethink. It probably should only display layers where the size differs between the two images. If there are no differences and it's an exact match I'm not sure what the expected output should be. Let me know if you have some ideas.
There was a problem hiding this comment.
I think for this case, it would be fine to just not display the result at all if the two sizes are identical. Since we're looking for diffs, this is the exact opposite of a diff, so we can just omit it from the results. Then the output will just have a list of the layers that have actual size differences. Does that make sense?
differs/size_diff.go
Outdated
| } | ||
|
|
||
| diff := util.EntryDiff{ | ||
| Name: strconv.Itoa(index), |
There was a problem hiding this comment.
it would be cool to use the layer digest here instead of just a number so we can print that to the user. do you think you might be able to add that?
I don't think it would be much more than just adding a Digest field to the image_utils.Layer struct, then calling layer.Digest() here when we're creating the layers. then you could add that into the diff struct and in the template that gets printed.
There was a problem hiding this comment.
Good idea. I'll have a go at adding this.
|
@nkubala Thanks for your review! I'll wait for guidance on a few points that I've outlined in the comments before making further changes. |
|
@peter-evans sorry for the delay on this one! I added a few comments, just a few changes need to be made here and this should be good to go 👍 |
|
@nkubala Thanks for the feedback! I've modified the code accordingly. |
This feature is a size analyzer for images and layers.
It aims to satisfy the request made in issue #236.
Adds two new types:
--type=size--type=sizelayer