Skip to content
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

validate: remove autodetect for type and add require limit #184

Merged
merged 2 commits into from
Jan 24, 2018

Conversation

Mashimiao
Copy link

  1. spec says any extra fields in json file can be ignored, so I think
    we can't clearly distingish which type the text file is, we'd better remove
    autodetect for text files.
  2. without audotdetect, we must requrie to user to set file type for validation

Signed-off-by: Ma Shimiao [email protected]

@Mashimiao
Copy link
Author

@opencontainers/image-tools-maintainers PTAL

@Mashimiao
Copy link
Author

ping @coolljt0725 @xiekeyang @q384566678 @stevvooe

@zhouhao3
Copy link

I think in create and unpack also need to make the appropriate changes.

@Mashimiao
Copy link
Author

create and unpack doesn't have text file type autodetect problem. Currently, whether type is required or not does not break their function. Let's solve validate's problem first. Then if necessary, we can make unified changes in another PR.

@zhouhao3
Copy link

zhouhao3 commented Sep 20, 2017

LGTM

This can also solve the problem in #62 . Wait for other's opinion.

Approved with PullApprove

@@ -22,7 +22,7 @@ oci-image-tool validate \- Validate one or more image files
Only applicable if type is image or imageLayout.

**--type**=""
Type of the file to validate. If unset, oci-image-tool will try to auto-detect the type. One of "imageLayout,image,imageZip,manifest,imageIndex,config"
Type of the file to validate. One of "imageLayout,image,manifest,imageIndex,config"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we set a default type if --type is unset? It seems odd if it doesn't work without any options.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea. Because --type is a little special, it's a must be required option but there is not a value could be used for all files, that means it's almost impossible to find a suitable default value.
So, I think let it to be unset will be better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is not a value could be used for all files, that means it's almost impossible to find a suitable default value.

Default is not a value for all files, it's value are most common. From that I know, it's not a good design from user perspective if a cli requires a option .

And I tested the patch

lei@ubuntu:~/images$ oci-image-tool validate busybox/
--type must be set
lei@ubuntu:~/images$ oci-image-tool validate --type imageLayout busybox/
1 errors detected:
busybox/: type "imageLayout" unimplemented
lei@ubuntu:~/images$ oci-image-tool validate --type image busybox/
autodetected image file type is: imageLayout
oci-image-tool: reference "latest": OK
busybox/: OK
Validation succeeded

Is the output autodetected image file type is: imageLayout necessary and why use --type imageLayout doesn't work but the autodetected image type is imageLayout?`

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry.... that's my fault. I should remove imageLayout from help and manpage.
In my design, users can only specify image,manifest,imageIndex,config to --type.
If we get that users want to validate an image, then we try to detect its type, may be a layout, tar file or zip file.
As you mentioned at #141 (comment), I try to use imageType to handle all image format.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, removed imageLayout. PTAL

@zhouhao3
Copy link

Also need to fix completions/bash/oci-image-tool.

@Mashimiao
Copy link
Author

Mashimiao commented Sep 21, 2017 via email

@zhouhao3
Copy link

In general I agree this PR. @coolljt0725, WDYT?

@Mashimiao
Copy link
Author

@opencontainers/image-tools-maintainers PTAL

@zhouhao3
Copy link

zhouhao3 commented Oct 12, 2017

LGTM

ping @coolljt0725 @xiekeyang PTAL

Approved with PullApprove

@zhouhao3
Copy link

ping @coolljt0725 @xiekeyang

@Mashimiao
Copy link
Author

ping @coolljt0725

@coolljt0725
Copy link
Member

ping @opencontainers/image-tools-maintainers @wking PTAL. I'm not quite sure about this.

@zhouhao3
Copy link

ping @opencontainers/image-tools-maintainers

@Mashimiao
Copy link
Author

@opencontainers/image-tools-maintainers PTAL

@vbatts
Copy link
Member

vbatts commented Nov 9, 2017

@Mashimiao is the autodetect causing issues? I'm am all for reducing magic-detection, but if this is not causing issues, then what is the issue?

@Mashimiao
Copy link
Author

Autodetect really causes issue. But I think that happens just when we use it for text file detection.
If we specified to validate the whole image not image files, it can easily and clearly find out the type of image. If the image is a directory, it must be a imageLayout. If not a directory, we can find out it content type and call validation function for that type.
So, I just removed the autodetec for image files(like index, config files), but retain the autodetect for image type.

@Mashimiao
Copy link
Author

ping @vbatts


**--type**=""
Type of the file to validate. If unset, oci-image-tool will try to auto-detect the type. One of "imageLayout,image,imageZip,manifest,imageIndex,config"
Type of the file to validate. One of "image,manifest,imageIndex,config"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An image isn't the layout. The layout is the collection of resources that can make up an image. For example, I can still have an image and not have a layout.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agree. And as you said, layout makes up an image. If we have a layout then we have an image. The point is image may be stored in different file types.
But if you don't have a layout, then it will not be a valid OCI image.
So, how do you think we should change the help info?

zhouhao and others added 2 commits January 24, 2018 20:04
Signed-off-by: zhouhao <[email protected]>
1. spec says any extra fields in json file can be ignored, so I think
we can't clearly distingish which type the text file is, we'd better remove
autodetect for text files.
2. without audotdetect, we must requrie to user to set file type for validation

Signed-off-by: Ma Shimiao <[email protected]>
@zhouhao3
Copy link

zhouhao3 commented Jan 24, 2018

LGTM

Approved with PullApprove

@zhouhao3 zhouhao3 merged commit 6539f0e into opencontainers:master Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants