-
Notifications
You must be signed in to change notification settings - Fork 247
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
Infer predefined annotations when pushing to registry #2618
Conversation
Also linked #2259 |
crates/oci/src/client.rs
Outdated
@@ -136,14 +148,15 @@ impl Client { | |||
locked: LockedApp, | |||
reference: impl AsRef<str>, | |||
annotations: Option<BTreeMap<String, String>>, | |||
predefined_annotations: PredefinedAnnotations, |
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.
The fact that annotations
is an option of a map and predefined_annotations
is an enum is somewhat confusing here; how much of this behavior is an external consumer of this API supposed to understand?
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.
@radu-matei I am gazing at this and struggling to think of a better design. I don't want to put it on consumers to construct the predefined annotations, so it shouldn't be a map. So I am having difficulty getting away from the "I want the predefined annotations or I don't" flag. I would value suggestions. Or is it as simple as "we should rename the flag to infer_predefined_annotations
or something, so it sounds like an action instead of a set" perhaps?
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.
@radu-matei I tried some renaming - if that doesn't improve matters then I'd value suggestions - thanks!
src/commands/registry.rs
Outdated
&app_file, | ||
&self.reference, | ||
annotations, | ||
PredefinedAnnotations::Infer, |
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.
So the behavior here is: always infer, and if the user provided any annotation explicitly, it takes priority over the inferred value??
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 the user provides one of the annotations that would be inferred explicitly, then the explicit value takes precedence - see the comment on the enum.
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.
Nice, thanks Ivan!
Not able to find a convention after a brief search. I think the comma-separated string as you've initially added seems a sane way to go for now. |
b65d224
to
f1bc4d5
Compare
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
f1bc4d5
to
3b54e14
Compare
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.
🎸👨🎤 🔈 LGTM+11!
Fixes #2612 (or at least part of it).
I'm not sure the best way to test this in CI, but trying it locally it seems promising:
The OCI
authors
annotation is a string, whereas Spin allows an array: the PR comma-space-concatenates them, e.g."The Great Oz, Dorothy Gale"
. OCI describes the field as a freeform string but if there's a common community convention then I can adopt that.This PR does not provide a way for a user to suppress inference but the infrastructure is there in case we want to add an option, or for programmatic use (e.g. reproducibility).