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

Switch from scratch to empty #1068

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

sudo-bmitch
Copy link
Contributor

@sudo-bmitch sudo-bmitch commented May 26, 2023

Here's an attempt to resolve #1067. I originally picked "filler" for the new name, but the community prefers "empty".

tianon
tianon previously approved these changes May 26, 2023
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

Generally in favor, and IMO you've picked a suitable replacement name - I've noted a few minor nits that I don't feel super strongly about. 👍

manifest.md Outdated Show resolved Hide resolved
manifest.md Outdated Show resolved Hide resolved
manifest.md Outdated Show resolved Hide resolved
manifest.md Outdated Show resolved Hide resolved
manifest.md Outdated Show resolved Hide resolved
@sudo-bmitch
Copy link
Contributor Author

Generally in favor, and IMO you've picked a suitable replacement name - I've noted a few minor nits that I don't feel super strongly about. +1

Thanks for the review! PR updated.

tianon
tianon previously approved these changes May 26, 2023
@rchincha
Copy link

"application/vnd.oci.empty.v1+json"

^ why complicate this needlessly?

@tianon
Copy link
Member

tianon commented May 28, 2023

I'm personally not a fan of "empty" because it's still ambiguous whether it's an actual layer or not (and this isn't intended to be a "proper" layer), so I quite like the proposed "filler" instead.

@rchincha
Copy link

I'm personally not a fan of "empty" because it's still ambiguous whether it's an actual layer or not (and this isn't intended to be a "proper" layer), so I quite like the proposed "filler" instead.

The goal IINM is to point out that it is a special thing, called something reasonable and best left alone. Do you foresee an "empty" something that folks would actually want to unmarshal/interpret? The very fact that this media-type is present means that this is no longer a "regular" image?

@tianon
Copy link
Member

tianon commented May 28, 2023 via email

@rchincha
Copy link

As per image-spec media-types - https://github.com/opencontainers/image-spec/blob/main/media-types.md,
layers are application/vnd.oci.image.layer.* and configs are application/vnd.oci.image.config.* things.

Scratch is a special thingie which is neither a layer nor a config and hence application/vnd.oci.scratch.v1+json

If one really wanted a scratch layer, I would have called application/vnd.oci.image.layer.scratch.*

We simply should have documented this better, saying there is no reason to use this except to mutate this container image into an "artifact".
https://github.com/opencontainers/image-spec/blob/main/specs-go/v1/manifest.go#L43

Reading all the conversations earlier - the term scratch appears as a tag, as an emptyfs layer, etc etc
How does renaming it to something else prevent this? "image-spec now has a type to refer to empty thingies called filler, let's use that"??

Maybe we are trying to get away from a certain mental model.

@sudo-bmitch
Copy link
Contributor Author

I'm not overly worried about misuse, but am concerned that there's an assumption the scratch blob is tightly connected to the scratch pseudo image. Since the pseudo image (FROM scratch) came first, I think it makes sense to name this something different, so that the media type we're adding isn't considered a recommendation by OCI for how the scratch pseudo image gets created.

// MediaTypeScratch specifies the media type for an unused blob containing the value `{}`
MediaTypeScratch = "application/vnd.oci.scratch.v1+json"
// MediaTypeFiller specifies the media type for an unused blob containing the value `{}`
MediaTypeFiller = "application/vnd.oci.filler.v1+json"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe:

Suggested change
MediaTypeFiller = "application/vnd.oci.filler.v1+json"
MediaTypeFiller = "application/vnd.oci.image.filler.v1+json"

Copy link
Member

Choose a reason for hiding this comment

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

Looking over the full list again, this does feel like a more consistent choice with our existing media types, but I don't feel really strongly either way. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel strongly on this either. It's more consistent to call it "image" since it's defined in the image spec and I don't want to step across to other specs. But if we start to define media types in the image-spec that are not an "image", then this new media type feels like it spans use cases.

Copy link
Member

@sajayantony sajayantony left a comment

Choose a reason for hiding this comment

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

Again, one of those things that should go away maybe 2.0 and be deprecated at some point when config and blobs are optional. I honestly liked the scratch name but understand why it could get confusing with FROM scratch

@rchincha
Copy link

OCI specs also have their share of filler episodes?

@jonjohnsonjr
Copy link
Contributor

jonjohnsonjr commented May 31, 2023

I don't love filler. I'd prefer empty or even artifact.

@tianon
Copy link
Member

tianon commented May 31, 2023

@jonjohnsonjr I'm not touching that last suggestion (and for your sake, I'm going to assume it was made in jest 😅), but I'm happy to give up my own weakly held opinion on empty in the face of more maintainer opinions, especially if it helps move this 👍

@rchincha
Copy link

Along with copious documentation that "this is not what you think it is" ... etc

@sudo-bmitch
Copy link
Contributor Author

My hesitation on "empty" was how the phrasing isn't always clear whether an "empty descriptor" doesn't contain any fields at all, or contains the fields pointing to a blob of 0 length, or points to a blob with the {} content. E.g. we would say things like:

This MUST be set when config.mediaType is set to the empty value.

And we would define an EmptyDescriptor value which isn't actually empty.

However, like tianon, my opinion here is weakly held and I'm happy to change this to whatever gains consensus.

@rchincha
Copy link

rchincha commented May 31, 2023

And we would define an EmptyDescriptor value which isn't actually empty.

as application/octet-stream perhaps not, but as "...+json" yes?

@sudo-bmitch
Copy link
Contributor Author

And we would define an EmptyDescriptor value which isn't actually empty.

as application/octet-stream perhaps not, but as "...+json" yes?

It's the difference between the content the descriptor points to being empty vs the descriptor itself being empty (which includes a media type, size, digest, and data field). We both understand what we're trying to say, but I'm worried about how things get misinterpreted by those that don't have the history and context behind this.

@rchincha
Copy link

rchincha commented Jun 1, 2023

but I'm worried about how things get misinterpreted by those that don't have the history and context behind this.

I see this as a gross failure in documentation (and perhaps education). Should such uninformed/partially informed concerns force a modification of the spec?

FROM scratch for example is a user-facing concept - everyone writes Dockerfiles. However, IINM, I cannot build an image with just that line. scratch media-type is/was internals and more a concern of tool developers, who in this case happened to misinterpret a unreleased version of the spec.

I am pointing out the shortcomings of the process, the mnemonic itself, like others, not a strong opinion.

@tianon
Copy link
Member

tianon commented Jun 1, 2023 via email

@rchincha
Copy link

rchincha commented Jun 1, 2023

If you'd like to make a PR to improve the documentation, please feel free. That seems orthogonal to me, and improved documentation would be valuable either way.

Fair, complaining is easy, action is hard.
#1070

@sajayantony sajayantony self-requested a review June 1, 2023 17:12
Copy link
Member

@sajayantony sajayantony left a comment

Choose a reason for hiding this comment

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

Sounds like there was less love towards filler and renaming this to indicate this is an "Empty JSON Descriptor" or something along application/vnd.oci.empty+json @tianon @rchincha

@sudo-bmitch sudo-bmitch changed the title Switch from scratch to filler Switch from scratch to empty Jun 1, 2023
specs-go/v1/mediatype.go Outdated Show resolved Hide resolved
specs-go/v1/manifest.go Outdated Show resolved Hide resolved
specs-go/v1/manifest.go Outdated Show resolved Hide resolved
Signed-off-by: Brandon Mitchell <git@bmitch.net>
@sudo-bmitch
Copy link
Contributor Author

@rchincha thanks. I've updated Empty to EmptyJSON and moved the descriptor definition into descriptor.go. We've been naming all media types to start with MediaType, so I used MediaTypeEmptyJSON and did the same with the DescriptorEmptyJSON. The v1 I in the media type I've been keeping from the scratch commit before, and it aligns with all other media types we're defining.

@@ -10,7 +10,7 @@ The following media types identify the formats described here and their referenc
- `application/vnd.oci.image.layer.v1.tar`: ["Layer", as a tar archive](layer.md)
- `application/vnd.oci.image.layer.v1.tar+gzip`: ["Layer", as a tar archive](layer.md#gzip-media-types) compressed with [gzip][rfc1952]
- `application/vnd.oci.image.layer.v1.tar+zstd`: ["Layer", as a tar archive](layer.md#zstd-media-types) compressed with [zstd][rfc8478]
- `application/vnd.oci.scratch.v1+json`: [Scratch blob](manifest.md#example-of-a-scratch-config-or-layer-descriptor)
- `application/vnd.oci.empty.v1+json`: [Empty for unused descriptors](manifest.md#guidance-for-an-empty-descriptor)
Copy link

Choose a reason for hiding this comment

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

If additional "strong" language/documentation advisable here, now would be a great time.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@sajayantony sajayantony left a comment

Choose a reason for hiding this comment

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

@vbatts make the original PR for scratch so might be good to get inputs.

@sajayantony sajayantony requested a review from tianon June 8, 2023 17:23
@tianon tianon merged commit f8b2ca8 into opencontainers:main Jun 8, 2023
4 checks passed
@sudo-bmitch sudo-bmitch deleted the pr-rename-scratch branch June 8, 2023 17:58
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.

Rename the scratch descriptor
6 participants