Skip to content

Switch from scratch to empty#1068

Merged
tianon merged 1 commit intoopencontainers:mainfrom
sudo-bmitch:pr-rename-scratch
Jun 8, 2023
Merged

Switch from scratch to empty#1068
tianon merged 1 commit intoopencontainers:mainfrom
sudo-bmitch:pr-rename-scratch

Conversation

@sudo-bmitch
Copy link
Copy Markdown
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
Copy Markdown
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. 👍

@sudo-bmitch
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

^ why complicate this needlessly?

@tianon
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Member

tianon commented May 28, 2023 via email

@rchincha
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

OCI specs also have their share of filler episodes?

@jonjohnsonjr
Copy link
Copy Markdown
Contributor

jonjohnsonjr commented May 31, 2023

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

@tianon
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@sudo-bmitch
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Member

tianon commented Jun 1, 2023 via email

@rchincha
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Signed-off-by: Brandon Mitchell <git@bmitch.net>
@sudo-bmitch
Copy link
Copy Markdown
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.

- `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
Copy Markdown
Contributor

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
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
@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