Skip to content

Conversation

@Mashimiao
Copy link

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

@Mashimiao
Copy link
Author

ping @opencontainers/image-tools-maintainers any comments?

@zhouhao3
Copy link

zhouhao3 commented Aug 2, 2017

I think you need to add TypeImageZip in validateTypes and other places. Also need to modify the man file.

@zhouhao3
Copy link

zhouhao3 commented Aug 2, 2017

And please rebase and modify.

Ma Shimiao added 2 commits August 22, 2017 10:13
@Mashimiao Mashimiao force-pushed the autodetect-add-more-type branch from 3515c80 to c02cd0c Compare August 22, 2017 02:25
@Mashimiao
Copy link
Author

rebased

@Mashimiao Mashimiao force-pushed the autodetect-add-more-type branch from c02cd0c to 01eed4b Compare August 22, 2017 06:22
@zhouhao3
Copy link

zhouhao3 commented Aug 22, 2017

LGTM

Approved with PullApprove

@Mashimiao
Copy link
Author

ping @coolljt0725 @xiekeyang

Signed-off-by: Ma Shimiao <[email protected]>
@Mashimiao Mashimiao force-pushed the autodetect-add-more-type branch from 01eed4b to 0b12f31 Compare August 22, 2017 10:10
@zhouhao3
Copy link

zhouhao3 commented Aug 22, 2017

LGTM

Approved with PullApprove

@zhouhao3
Copy link

ping @coolljt0725 @xiekeyang @stevvooe

@coolljt0725
Copy link
Member

coolljt0725 commented Sep 4, 2017

This is not work for me.

$ oci-image-tool validate busybox.zip
1 errors detected:
busybox.zip: validation failed: file does not exist

I debugged and guess it should be something wrong with zipWalker (https://github.com/opencontainers/image-tools/pull/141/files#diff-52676de86adecced88d6dc21eb58fbcaR222), the get method is same with the tarWalker here, but I think it should be different, because when open a tar archive with vim it shows like this

" tar.vim version v29
" Browsing tarfile /home/lei/images/busybox.tar
" Select a file with cursor and press ENTER

busybox/
busybox/oci-layout
busybox/index.json
busybox/blobs/
busybox/blobs/sha256/
busybox/blobs/sha256/8ac8bfaff55af948c796026ee867448c5b5b5d9dd3549f4006d9759b25d4a893
busybox/blobs/sha256/8baf43d43a34a0e6649c254b0200c2406fc40a501a852ba51a86ac3672dc0441
busybox/blobs/sha256/40a114053d955a2b80ee2cf6e13410b28b59594ceee9036b41e12c42d3e16615

but open a zip archive with vim, it shows like this

" zip.vim version v27
" Browsing zipfile /home/lei/images/busybox.zip
" Select a file with cursor and press ENTER

blobs/
index.json
oci-layout

I think that's why this can't work?

Does this work for you ? or Am I missing something?

@Mashimiao
Copy link
Author

I think that maybe because you didn't put oci-layout, blobs and index.json into the root of the archive. A valid archive of image should have those at the root of the archive, not nested within a top-level directory as busybox.

@zhouhao3
Copy link

zhouhao3 commented Sep 4, 2017

It works for me:

➜  oci-image-tool validate ubuntu.zip 
oci-image-tool: reference "latest": OK
ubuntu.zip: OK
Validation succeeded

@coolljt0725
Copy link
Member

sorry, It seems something wrong with my zip archive, it works with another zip archive

$ oci-image-tool validate busybox.zip
oci-image-tool: reference "latest": OK
busybox.zip: OK
Validation succeeded

@coolljt0725
Copy link
Member

Can we avoid adding a new TypeImageZip type and just use TypeImage, let the TypeImage to handle the different archive format? An zip format archive is also a file for image tool. And in this way, I think we can reduce some duplication code. WDYT?

@Mashimiao
Copy link
Author

Letting TypeImage to handle different archive is a good option. But I think we have to do a lot changes to reach that.
Based on current structure, add TypeImageZip is the best choice, and rebuild the structure has less to do with the subject - adding support zip and rar.
So, how about making a follow-up PR to implement that after this PR landed? I can submit such PR later.

@coolljt0725
Copy link
Member

@Mashimiao ok. LGTM wait for other's opinion @opencontainers/image-tools-maintainers

@xiekeyang
Copy link
Contributor

xiekeyang commented Sep 7, 2017

LGTM

Approved with PullApprove

@zhouhao3 zhouhao3 merged commit 386cd33 into opencontainers:master Sep 7, 2017
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.

4 participants