-
Notifications
You must be signed in to change notification settings - Fork 658
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
Update artifacts-guidance based on proposed changes #1100
Conversation
Update guidance based on proposed changes https://opencontainers.org/posts/blog/2023-07-07-summary-of-upcoming-changes-in-oci-image-and-distribution-specs-v-1-1/ Signed-off-by: Dasith Wijesiriwardena <[email protected]>
Update artifacts-guidance based on proposed changes
I feel this guidance seems appropriate for v1.1 GA. Related discussion We might need to update in the spec.md as well since it recommends setting the config mediaType - Lines 170 to 174 in 82d42b1
Pivoting to use the new field as forward looking guidnace would encourage new tools to move to this but it it important that we call out that config.mediaType is equally supported and all consumers should fully support that. |
If I'm reading this right, the main change here is that artifacts are strongly discouraged (very close to MUST language) from using the config blob for anything except the empty object? For artifacts that don't have a use for it, that makes sense, but I'm sure there's a use case out there where that doesn't make sense. The first that comes to my mind is something image shaped that isn't an image (like WASM/WASI images), so I don't think I agree that we ought to so strongly advise against this at the spec level (and certainly don't feel we ought to go as far as forbidding it, although this doesn't appear to formally forbid it). Perhaps this was not the intent and we just need to tweak the wording somehow? |
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.
This is not the guidance I've been giving. It's applying a single use case to all uses cases, breaking existing artifacts (like helm charts). The advice to use the artifactType
field only applies to those situations where an artifact does not have blob content for the config.mediaType
. Perhaps the spec needs a flow chart for implementations to decide how to format their artifacts?
@tianon Not at all. It is to encourage the use of the new top level I was going on the advice from this link though https://opencontainers.org/posts/blog/2023-07-07-summary-of-upcoming-changes-in-oci-image-and-distribution-specs-v-1-1/
re: requested changes
Happy to do a decision tree. @sudo-bmitch @sajayantony Is "artifact does not have blob content" the only deciding factor? edit: Also, with regards to artefacts with blob content, is the advice going forward to use the |
I don't believe this is a case of "we made a new thing, throw out all the old things". The guidance avoided picking one over another because the appropriate format depends on each use case, and there are many use cases that were created before we added the |
If you prefer I can add a decision tree in a mermaid diagram format in this page and keep the extended text based guidance for the image spec page? |
I've been trying to keep all of the image manifest guidance in the The alternative is to move all of the image manifest guidance into the Regarding mermaid diagrams, I don't believe we have any of them in the existing documentation. We would need to be sure pandoc supports them, or use the dot -> png conversion like we use for other images. |
Signed-off-by: Dasith Wijesiriwardena <[email protected]>
@sudo-bmitch I took your changes from #1101 and added examples aligned for each use case as well. I'm not sure which one of these PRs will go before the other but feel free to take my last commit to your branch if yours is the one that lands. |
In addition to the linter issues, the code block headers use the media type for testing, so we'll want those back. |
I've fixed the linting issues and added the url encoded titles for the |
Signed-off-by: Dasith Wijesiriwardena <[email protected]>
Signed-off-by: Dasith Wijesiriwardena <[email protected]>
I've added |
Anything else required here @sudo-bmitch @sajayantony ? |
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.
(temporary explicit NACK based on discussion on #1101 whose changes were copied here)
Signed-off-by: Brandon Mitchell <[email protected]>
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.
This would need a rebase on the output of #1101. I don't have a strong opinion on whether the examples should go in the decision tree or after, but if they are included in the tree, I think we want to make it clear that the tree is guidance, and the code block is an example. People have a way of interpreting examples as specs themselves, and over fitting their implementations.
Signed-off-by: Dasith Wijesiriwardena <[email protected]>
Signed-off-by: Dasith Wijesiriwardena <[email protected]>
Closing this as changes have been applied in #1101 and oras-project/oras-www#248. Thanks to everyone involved. |
Update guidance based on proposed changes https://opencontainers.org/posts/blog/2023-07-07-summary-of-upcoming-changes-in-oci-image-and-distribution-specs-v-1-1/
This is on the assumption that
image.artifactType
field is now the preferred way to denote artefact types that are not images.