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.

Sign up for GitHub

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

Jump to bottom

Modify the ref information #89

Closed
wants to merge 1 commit into from
Closed

Modify the ref information #89

wants to merge 1 commit into from

Conversation

zhouhao3
Copy link

@zhouhao3 zhouhao3 commented Nov 28, 2016 •
edited
Loading

According to some norms can be seen, ref can also point to the manifest list, here only to modify some of the error message, some code error will continue to update in #51 .

I think it's more appropriate to change ref to tag.

Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

coolljt0725
coolljt0725 reviewed Nov 28, 2016
View reviewed changes
image/image.go Outdated
@@ -120,7 +120,7 @@ func unpack(w walker, dest, refName string) error {
return err
}

if err = ref.validate(w, validRefMediaTypes); err != nil {
if err = ref.validate(w, []string{v1.MediaTypeImageManifest}); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

the ref could refer to manifest or manifest list, I don't think we need this change.

Copy link
Author

Choose a reason for hiding this comment

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

But in this place must be manifest

Copy link
Member

@coolljt0725 coolljt0725 Nov 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so. If the ref of an OCI image refer to a manifest list, and the the manifest list refer to a manifest, the findManifest below are supposed to get the manifest from the manifest list(the pr #51 your are woking on are doing this). With your changes, this process are break.

Copy link
Author

@zhouhao3 zhouhao3 Nov 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

manifest list points to only a few special manifests, just specify its platform, there is no other property requirements. I do not think this is what you want to get, and in the instructions, ref can only point to the manifest.

Copy link
Author

Choose a reason for hiding this comment

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

In the pr #51 ,Only in the validation function need to confirm the manifest and manifestlist, and I just change the unpack and create-runtime-bundel function

Copy link
Member

Choose a reason for hiding this comment

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

manifest list points to only a few special manifests, just specify its platform, there is no other property

I don't know if you understand the manifest list correctly :). How could you unpack a OCI image if the ref( for more information about ref https://github.com/opencontainers/image-spec/blob/master/image-layout.md) points to manifest list ? and how do you know the ref is always point to a manifest ?

Copy link
Author

Choose a reason for hiding this comment

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

You are right, I was misled by some wrong information, and now I have this information modified.

@zhouhao3 zhouhao3 changed the title image.go:Modify the ref validation range image.go:Modify the ref regulatory information Nov 29, 2016
@coolljt0725
Copy link
Member

@q384566678 image-tool can't handle manifest list at the moment, let's waiting the image-tool for adding this function and then let's has these changes in this pr, WDYT?

@zhouhao3
Copy link
Author

let's waiting the image-tool for adding this function and then let's has these changes in this pr, WDYT?

I increase the corresponding function in the pr #51 , because of some rely on pr #51 function, so can't update in the pr.

@zhouhao3
Copy link
Author

@xiekeyang @coolljt0725 @vbatts @stevvooe PTAL

@coolljt0725
Copy link
Member

let's make the image-tool can handle manifest list first then update the docs. close at the moment and will reopen once the image-tool can handle manifest list

@coolljt0725 coolljt0725 closed this Feb 7, 2017
@xiekeyang xiekeyang mentioned this pull request Feb 16, 2017
Merged
@zhouhao3 zhouhao3 reopened this Jul 20, 2017
@zhouhao3
Copy link
Author

Since the verification of the index has been improved in #51 , so I reopen this.

@zhouhao3 zhouhao3 force-pushed the ref-validate branch 3 times, most recently from c9ccc11 to 18ea452 Compare July 20, 2017 05:40
@zhouhao3 zhouhao3 changed the title image.go:Modify the ref regulatory information man: Modify the ref information Jul 20, 2017
Mashimiao
Mashimiao reviewed Jul 21, 2017
View reviewed changes
man/oci-image-tool-create.1.md Outdated
@@ -18,7 +18,7 @@ runtime-spec-compatible `dest/config.json`.
Print usage statement

**--ref**=""
The ref pointing to the manifest of the OCI image. This must be present in the "refs" subdirectory of the image. (default "v1.0")
The ref pointing to the manifest or the image index of the OCI image. This must be present in the "refs" subdirectory of the image. (default "v1.0")
Copy link

Choose a reason for hiding this comment

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

I think refs directory does not exist any more according to the spec

Copy link
Author

Choose a reason for hiding this comment

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

updated. And I think it's more appropriate to change ref to tag.

@zhouhao3 zhouhao3 force-pushed the ref-validate branch 3 times, most recently from 91a3531 to 02c6e21 Compare July 21, 2017 07:31
@zhouhao3 zhouhao3 changed the title man: Modify the ref information change ref option to tag Jul 21, 2017
@zhouhao3 zhouhao3 changed the title change ref option to tag change ref option to tag and fix info Jul 21, 2017
Mashimiao
Mashimiao reviewed Jul 21, 2017
View reviewed changes
man/oci-image-tool-create.1.md Outdated
**--ref**=""
The ref pointing to the manifest of the OCI image. This must be present in the "refs" subdirectory of the image. (default "v1.0")
**--tag**=""
The tag pointing to the manifest or the image index of the OCI image. his must be matching 'org.opencontainers.image.ref.name' annotations in index.json. (default "v1.0")
Copy link

@Mashimiao Mashimiao Jul 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The spec always uses ref, and I prefer to ref. And ref must be pointing to a manifest, will not be an image index, we can get the manifest via image index. So I think the change is not correct.

@zhouhao3 zhouhao3 changed the title change ref option to tag and fix info Modify the ref information Jul 21, 2017
Modify the ref information
afa6f23 
Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
@Mashimiao Mashimiao mentioned this pull request Aug 4, 2017
Merged
@zhouhao3
Copy link
Author

Because #169 has been merged, so close this pr.

@zhouhao3 zhouhao3 closed this Jan 24, 2018
@zhouhao3 zhouhao3 deleted the ref-validate branch January 24, 2018 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coolljt0725 coolljt0725 coolljt0725 left review comments

@Mashimiao Mashimiao Mashimiao left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhouhao3 @coolljt0725 @Mashimiao