feat: add support for setting the pushed oci image manifest annotations#2254
Conversation
1656611 to
12b60ea
Compare
itowlson
left a comment
There was a problem hiding this comment.
Thanks for this! Someone else will review the OCI side; I just had a couple of naming nits for the command code. We really appreciate the contribution!
src/commands/registry.rs
Outdated
| use spin_oci::Client; | ||
| use std::{io::Read, path::PathBuf, time::Duration}; | ||
|
|
||
| use super::new::ParameterValue; |
There was a problem hiding this comment.
This type is used for template parameters. I realise that today it happens to have the right shape and parsing behaviour that you need for annotations, but we should not bind the two together. We should either:
- Create a new type defined by shape and behaviour (a la
EqualsSeparatedStringPair) in a shared location, and use that in both places; or - Create an Annotation type in this module, and use that here.
There was a problem hiding this comment.
I found a common type at use spin_common::arg_parser::parse_kv, so I've rewritten to code to use it.
src/commands/registry.rs
Outdated
| spin_build::build(&app_file, &[]).await?; | ||
| } | ||
|
|
||
| let annotations = self.annotations.map(|parameters| { |
There was a problem hiding this comment.
The names parameters and parameter in this section are misleading.
There was a problem hiding this comment.
Ah, indeed! In the meantime, this code no longer exists. Please see the new one.
2c8494b to
9ebda34
Compare
src/commands/registry.rs
Outdated
| if self.annotations.is_empty() { | ||
| None | ||
| } else { | ||
| Some(self.annotations.iter().cloned().fold( |
There was a problem hiding this comment.
nit: you can do this with self.annotations.iter().cloned().collect() - no need for the mutable HashMap and the fold - an iterator of tuples can be collect-ed to a HashMap. To me this seems more concise and idiomatic than the mutating fold, although what you have works and this is not a blocker.
There was a problem hiding this comment.
if there are duplicate --annotation arguments with the same key (e.g. --annotation a=a --annotation a=aa) won't that cause a panic?
There was a problem hiding this comment.
No, last one wins, same as with the fold.
There was a problem hiding this comment.
Oh nice! Thank you for clarifying this!
I've updated the code, please check it now.
vdice
left a comment
There was a problem hiding this comment.
LGTM, thanks @rgl!
Will hold off on merging in case any optional changes per #2254 (comment) come through.
|
@rgl sorry, I often forget this detail on new contributions: Thanks for completing the DCO sign-off; can you also please ensure that the commit(s) are GPG-signed? (When ready.) |
9ebda34 to
8c2c5f6
Compare
|
@vdice I think everything should be ready to go now :-) |
|
@vdice please do not yet merge this until project-zot/zot#2210 is addressed. I'm starting to think that we should have a way to set the image manifest annotations (what the current |
|
@rgl Would it be overkill to take the provided A question: Refreshing my knowledge with the config spec, I see that the config object has a (As an aside, I built on this PR to play around a bit and I'm not immediately seeing generic annotations added to the config layer (here) show up as intended in the manifest json... so there may be a bit of further work either in our oci client or the underlying oci-distribution crate. I haven't yet tried adding metadata to the |
|
Hi @rgl checking in on this one. Wondering what you were thinking re: #2254 (comment). Thanks! |
|
It looks like
"Config": {
"Hostname": "",
"Domainname": "",
"User": "65532:65532",
"AttachStdin": false,
"AttachStdout": false,
"AttachStderr": false,
"Tty": false,
"OpenStdin": false,
"StdinOnce": false,
"Env": [
"SSL_CERT_FILE=/etc/ssl/certs/ca-certificates.crt",
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
],
"Cmd": null,
"Image": "",
"Volumes": null,
"WorkingDir": "/",
"Entrypoint": [
"/manager"
],
"OnBuild": null,
"Labels": {
"org.opencontainers.image.created": "2024-02-19T16:28:43.720Z",
"org.opencontainers.image.description": "",
"org.opencontainers.image.licenses": "NOASSERTION",
"org.opencontainers.image.revision": "ff68ce40b1d5e202a717483297db0eaa3bbacdc5",
"org.opencontainers.image.source": "https://github.com/my-org/my-project",
"org.opencontainers.image.title": "my-project",
"org.opencontainers.image.url": "https://github.com/my-org/my-project",
"org.opencontainers.image.version": "main"
}
}, |
|
@vdice sorry for the radio silence, but I'm afraid I was not yet able to dedicate quality time to understand the OCI image-spec in more details to be able to answer. I'm still interested in looking into this, hopefully soon :-) |
|
@endocrimes do you known if that is aligned with the oci image-spec? I still didn't get a chance to spend quality time reading the specs, hopefully I can do it soon. |
|
@endocrimes sorry to hound but bark, bark |
endocrimes
left a comment
There was a problem hiding this comment.
I'm happy for this to land as is, and update later if needed for broader compatibility.
|
Thanks @endocrimes! @rgl thanks for your patience - it's been a crazy busy time - could you rebase and resolve the merge conflict please? Then we can land this. Thanks! |
Signed-off-by: Rui Lopes <rgl@ruilopes.com>
8c2c5f6 to
9ecb814
Compare
|
@itowlson no worries! I've rebased it. |
|
Thank you @rgl! Glad to have landed this and thanks again for sticking with it. |
This closes #2236.
You can see how GitHub Container Registry shows an image without annotations at:
https://github.com/rgl/spin-http-ts-example/pkgs/container/spin-http-ts-example/171707227?tag=0.2.0
And the one with annotations at:
https://github.com/rgl/spin-http-ts-example/pkgs/container/spin-http-ts-example/171719572?tag=0.0.0-test1
This is how I've pushed it the image with my local spin version: