Update version

[zhouhao3]

Update the image-spec version and fix the relevant code.

Signed-off-by: zhouhao [email protected]

[coolljt0725]

@q384566678 There is a huge change on v1.0.0-rc5 opencontainers/image-spec#533 . This changes the layout, would you mind to change image-tool correspond to the image-spec v1.0.0-rc5?

[zhouhao3]

@coolljt0725 I am making these corresponding changes, and I think it would be better to submit another change after this PR merge.

[zhouhao3]

@coolljt0725 I would like to ask, now in the absence of ref, how to use index.jsonto identify manifest? Is it by digest or something else?

[wking]

On Wed, Feb 22, 2017 at 11:10:05PM -0800, Zhou Hao wrote: @coolljt0725 I would like to ask, now in the absence of `ref`, how to use `index.json`to identify `manifest`? Is it by `digest` or something else?

By org.opencontainers.ref.name [1,2]. [1]: https://github.com/opencontainers/image-spec/blob/v1.0.0-rc5/annotations.md#pre-defined-annotation-keys [2]: https://github.com/opencontainers/image-spec/blob/v1.0.0-rc5/image-layout.md#indexjson-file

[zhouhao3]

@wking But annotation is optional field, and ref.name may also not exist with annotation. If there is no how to use it?

[wking]

On Thu, Feb 23, 2017 at 05:51:16PM -0800, Zhou Hao wrote: @wking But `annotation` is optional field, and `ref.name` may also not exist with annotation. If there is no how to use it?

A name-based API would not be able to access index entries which did not set org.opencontainers.ref.name. You could define any number of alternative APIs to access those entries (e.g. by offset in the index array), but I don't know if image-tools needs to bother with those.

[cyphar]

@q384566678 I am working on implementing it in umoci and once that's done that code will be merged with this repo. It would be a waste of time for you to implement it as well (since most of the walking code in this repo is going to swapped out for the CAS implementation in umoci). There are also some current discussions in the specification (opencontainers/image-spec#581) about refs.name.

[zhouhao3]

On ref.name and some other issues are not resolved in this PR, the PR just want to update the version, after some of the problems but also wait for it to merge and then resolved.
So, @coolljt0725 @cyphar @xiekeyang @stevvooe @vbatts PTAL

[cyphar]

@q384566678 You cannot "just update the version" -- the specification is not the Go structures included in specs-go. The most important part of the spec is what's contained in the Markdown files (the specs-go is just an implementation in Go of the structures defined in the spec).

So, "just updating the version" of the Go structures is actually making image-tools no longer compliant with the specification, because it no longer follows v1.0.0-rc4 (the mediaTypes and other version strings are different) and it doesn't follow v1.0.0-rc5 (because the actual layout is not compliant with the spec).

So, NACK. I'm also considering closing this because this code will come from umoci -- not to mention there are still open issues about refs.name that make implementation difficult at the moment.

[runcom]

btw, tests in skopeo are broken and we cannot update to RC5 https://travis-ci.org/projectatomic/skopeo/builds/205841968#L1476 I'm going to comment that test out @cyphar ....this can't just work

[cyphar]

@runcom I was planning on switching the OCI implementation in skopeo to use umoci's libraries (which are going to end up here) so that these transition periods would be much easier. But even if we merged this it still wouldn't work -- the code is simply not correct for v1.0.0-rc5 (Validate would fail).

I'm working on fixing up this project, but it's quite taxing because of the large amounts of things that now need to be spun out of umoci (while I would just prefer if we took umoci and made it an OCI project to make this less of a massive pain).

But yeah, sorry about this project being broken for v1.0.0-rc5 and it really sucks that it's blocking skopeo. But are you sure that you want to update to v1.0.0-rc5 when effectively no other tools support it yet (and not to mention there's still discussions happening in the spec about how the new referencing system is meant to work)? I'd prefer if we didn't disable tests that ensure that OCI round-trips are actually sane -- disabling that will almost certainly result in breakages in the future (as it has in the past).

[stevvooe]

LGTM

@runcom That doesn't look like a failed test. It looks like a build error.

[cyphar]

@stevvooe This PR does not correctly implement v1.0.0-rc5, so it breaks the spec support. The build failure is because I added tests to skopeo that use this project (and since skopeo has updated their vendored version of image-spec the build fails here).

Closing because it's not correct, should not be merged in its current state, and as far as I can tell @q384566678 doesn't appear to want to make it correct.

[stevvooe]

@cyphar He is updating a dependency of this project. What is not correct?

[runcom]

@cyphar we haven't yet merged the RC5 update and we likely won't for now, was just pointing out I found that and I'm commenting some tests in my PR. BTW, the docker/docker PR is updated to use RC5.

[stevvooe]

These changes are required, whether there are subsequent required changes or not. I don't see why this is closed.

[cyphar]

@stevvooe

He is updating a dependency of this project. What is not correct?

The dependency he is updating is image-spec. This project's job is to implement the spec. By updating to v1.0.0-rc5 without implementing the index.json changes, this project no longer implements the spec. This would be a problem, so it's not just a matter of "subsequent required changes" -- master would no longer implement the spec if this is merged as-is.

I don't see why this is closed.

As far as I can tell @q384566678 is not interested in implementing the required index.json changes. Since I'm already implementing them in umoci and they are going to be put into this project, duplicating effort like this doesn't make sense (all of the walking code is going to be removed and the validate code will be rewritten).

I closed it so it doesn't get merged in its current state.

[zhouhao3]

I think this PR can be shelved first (no need to close). We can wait until index.json is resolved, and then update the PR according to the change.

[cyphar]

I'm going to reiterate that this code is going to be replaced with the umoci/cas implementation. But if you really want to spend your time writing code that's going to be replaced, that's your prerogative.

[zhouhao3]

I am waiting for your results.

[stevvooe]

LGTM

@cyphar There is no mandate for master to implement the spec. Let's move the project forward. If you're depending on this project, it is too early.

[zhouhao3]

@coolljt0725 @xiekeyang PTAL

[cyphar]

sigh Okay. I'm a bit frustrated that we are happy about breaking compat with the spec -- especially since one of the jobs of this project is to validate images. Depending on this project is currently the only way of being somewhat confident that you are on the right track of implementing a valid OCI image handler.

[wking]

On Sat, Mar 04, 2017 at 06:41:10AM -0800, Aleksa Sarai wrote: I'm a bit frustrated that we are happy about breaking compat with the spec -- especially since one of the jobs of this project is to **validate** images.

A middle ground between master always targeting a released spec version [1] and master chasing the spec's master while allowing for (hopefully temporary) inconsistencies [2] is let these inconsistencies into master (e.g. by merging this PR as it stands), but then cleaning up those inconsistencies before you cut the next image-tools release. Maintaining a corpus of (in)valid images for the target spec version(s) [3] and marking broken tests would be an easy way to track outstanding inconsistencies to avoid cutting a release without addressing them. [1]: #121 (comment) [2]: #121 (comment) [3]: https://groups.google.com/a/opencontainers.org/d/msg/dev/GZ2q3jGlI0w/AUna584QCwAJ Subject: Re: Merging umoci and image-tools Date: Fri, 17 Feb 2017 10:15:01 -0800 Message-ID: <[email protected]>

[coolljt0725]

LGTM

[coolljt0725]

ping @opencontainers/image-tools-maintainers I think we need more LGTMs on this one

[cyphar]

@coolljt0725 No we don't, it has enough (2).

I've not given this an LGTM for the reasons outlined above, so I'm personally against this being merged (it doesn't accomplish anything meaningful aside from making master no longer implement the spec). But since this has 2 LGTMs, you can merge it.

[jonboulle]

posthumous -1 on this, +1 to cyphar's take. Why can't we just fix up the tooling as we bump the spec?

[zhouhao3]

Why can't we just fix up the tooling as we bump the spec?

I was trying to do this before, but there are some specific problems can not be solved, I am going to solve these problems, I will fix image-tools as soon as possible.

[lucab]

Ping, any update on this? image-tools has been in this state half-way between -rc4 and -rc5 since this PR landed one month ago; perhaps it is worthy to revert this PR while -rc5 is under work?

[stevvooe]

@jonboulle @lucab What is missing here? Could you file an issue identifying the gap?

Let's move forward rather than holding things back.

[coolljt0725]

@q384566678 hi, are you still working on make the image-tool work with -rc5?

[zhouhao3]

@coolljt0725 I'm doing it. I've been late for some time.