-
Notifications
You must be signed in to change notification settings - Fork 669
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
Conversation
There was a problem hiding this 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. 👍
075dc5b
to
4d585e1
Compare
Thanks for the review! PR updated. |
"application/vnd.oci.empty.v1+json" ^ why complicate this needlessly? |
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? |
Yes, one of the things that led to this rename proposal is folks doing
exactly that.
|
As per image-spec media-types - https://github.com/opencontainers/image-spec/blob/main/media-types.md, Scratch is a special thingie which is neither a layer nor a config and hence If one really wanted a scratch layer, I would have called We simply should have documented this better, saying there is no reason to use this except to mutate this container image into an "artifact". Reading all the conversations earlier - the term Maybe we are trying to get away from a certain mental model. |
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 ( |
specs-go/v1/mediatype.go
Outdated
// 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
MediaTypeFiller = "application/vnd.oci.filler.v1+json" | |
MediaTypeFiller = "application/vnd.oci.image.filler.v1+json" |
There was a problem hiding this comment.
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. 👍
There was a problem hiding this comment.
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.
There was a problem hiding this 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
OCI specs also have their share of filler episodes? |
I don't love |
@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 |
Along with copious documentation that "this is not what you think it is" ... etc |
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
And we would define an However, like tianon, my opinion here is weakly held and I'm happy to change this to whatever gains consensus. |
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. |
I see this as a gross failure in documentation (and perhaps education). Should such uninformed/partially informed concerns force a modification of the spec?
I am pointing out the shortcomings of the process, the mnemonic itself, like others, not a strong opinion. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4d585e1
to
d8096c8
Compare
Signed-off-by: Brandon Mitchell <[email protected]>
d8096c8
to
25fc553
Compare
@rchincha thanks. I've updated Empty to EmptyJSON and moved the descriptor definition into |
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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.
Here's an attempt to resolve #1067. I originally picked "filler" for the new name, but the community prefers "empty".