images: use mediatype helpers #9154
Conversation
|
Hi @neersighted. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Looks like a linter is complaining about an empty "else" branch (despite the comment in it); could either continue using a switch, or perhaps add a comment above the 
|
|
I've contained this to the |
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
d438583 to
62f621a
Compare
|
/ok-to-test |
| for _, desc := range eo.manifests { | ||
| switch desc.MediaType { | ||
| case images.MediaTypeDockerSchema2Manifest, ocispec.MediaTypeImageManifest: | ||
| if images.IsManifestType(desc.MediaType) { |
There was a problem hiding this comment.
Alternatively, you could keep a switch, and use the boolean checks in it (I personally find those to be slightly more readable as the condition is more "in plain sight"), but that's really a bike-shed, so just leaving the comment for consideration 😄
switch {
case images.IsManifestType(desc.MediaType):
// ...
case images.IsIndexType(desc.MediaType):
// ...
default:
// ...
}There was a problem hiding this comment.
I tend to prefer the if and braces style for relatively dense blocks like this; unless another review prefers this, I think I'd like to leave this how I have it.
By aligning with the
mediatypes.gohelpers, there is a single source of truth for the mediatype checks in theimages/package tree, and we can remove some case statements. By dropping case statements, branching logic can be simplified and made more obvious, with less nesting.