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

align with OCI artifact best practices #11120

Closed
wants to merge 1 commit into from

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Oct 23, 2023

What I did
better align with OCI artifact best practices (https://oras.land/docs/concepts/artifact/):

  • artifact blob layer is pushed with mediatype application/vnd.oci.image.layer.v1.tar
  • org.opencontainers.image.created annotation is set

(not mandatory) A picture of a cute animal, if possible in relation to what you did

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof requested review from neersighted, milas, a team, nicksieger, StefanScherer, ulyssessouza, glours and laurazard and removed request for a team October 23, 2023 13:56
@neersighted
Copy link
Member

neersighted commented Oct 23, 2023

I don't think that page ORAS represents best practices; in particular there is no need to use a layer tar mediaType when that is not actually the format of the data.

It's worth noting that they also show a custom layer mediaType in their "with config" example -- I suspect the overloading of the layer mediaType is merely an error in updating the page.

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (6ab41d6) 57.54% compared to head (f659896) 57.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11120      +/-   ##
==========================================
- Coverage   57.54%   57.44%   -0.10%     
==========================================
  Files         129      129              
  Lines       11272    11276       +4     
==========================================
- Hits         6486     6478       -8     
- Misses       4156     4166      +10     
- Partials      630      632       +2     
Files Coverage Δ
pkg/compose/publish.go 0.00% <0.00%> (ø)

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ndeloof
Copy link
Contributor Author

ndeloof commented Oct 23, 2023

@neersighted right, would you recommend a better "source of truth" ? I hardly can find documentation on OCI artifacts, Google 1st result being archived https://github.com/opencontainers/artifacts 😅

@neersighted
Copy link
Member

neersighted commented Oct 23, 2023

https://github.com/opencontainers/image-spec/blob/main/manifest.md#guidelines-for-artifact-usage

I'm also working on trying to make more of this obvious in the spec, e.g. at opencontainers/image-spec#1141. Reviews of that PR/suggestions for further content or structure improvements are welcome.

@ndeloof ndeloof closed this Oct 23, 2023
@neersighted
Copy link
Member

Oh! And if I wasn't clear -- I think it's still worth carrying the annotation change; just not the change to the layer mediaType.

@ndeloof
Copy link
Contributor Author

ndeloof commented Oct 23, 2023

sure, I plan to update this PR with a few other fixes, so making this one a draft

@ndeloof
Copy link
Contributor Author

ndeloof commented Oct 23, 2023

"reopen" action failing, will create another one 😅

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.

3 participants