-
Notifications
You must be signed in to change notification settings - Fork 153
feat: enable customization of annotations+labels #587
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
base: master
Are you sure you want to change the base?
feat: enable customization of annotations+labels #587
Conversation
Signed-off-by: Brendon Smith <bws@bws.bio>
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 think you can simplify this PR quite a bit by getting opt-out functionality via empty annotation values (some defaults like licenses can result in empty strings).
The only other related functionality that comes to mind is if someone were to want more control between index and manifest annotation assignment. I don't think there's an open issue with a use-case requesting such support however, so for now I'd just favor the opt-out via empty value approach, it'd reduce your code contribution quite a bit and simplify the UX for users instead of adding extra complexity.
BTW, it would have been helpful for review if you had used a separate commit for shifting the bulk of the README changes around before adding the YAML snippet (and preceding paragraph), as that'd have made the diff much simpler to follow and grok.
| with: | ||
| images: name/app | ||
| labels: | | ||
| name=org.opencontainers.image.url,enable=false |
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 am not sure about this syntax, would it make more sense to just opt-out via setting an empty value instead? Slight more terse, and doesn't introduce an alternative syntax.
I had suggested opt-out by setting an empty value in a recent issue I opened.
- Some of the default values can already produce an empty value, but are still applied (which also overrides the
LABELif present in aDockerfile) but I don't think there's a good reason to expect/want that to occur? - I'm not sure how often someone would want to set an empty annotation or label, I suppose for
LABELit might be to "unset" one that was inherited? If so they could still do that within theirDockerfile, so I don't think it's relevant fordocker/metadata-actionto support keeping empty annotations/labels to apply.
However my request would be a breaking change to anyone who did rely on setting empty values to override image labels.
An alternative could be a separate input to provide a list of default labels/annotations to exclude? They're all using the same namespace and defaults aren't configurable, so it's probably okay to take that approach instead too 🤷♂️ (then it avoids the risk of breaking anyone that might expect empty values to be supported + less verbose if disabling multiple labels)
| ### Default labels and annotations | ||
|
|
||
| The action will set the following default labels and annotations based on repository metadata: | ||
|
|
||
| - `org.opencontainers.image.title` | ||
| - `org.opencontainers.image.description` | ||
| - `org.opencontainers.image.url` | ||
| - `org.opencontainers.image.source` | ||
| - `org.opencontainers.image.version` | ||
| - `org.opencontainers.image.created` | ||
| - `org.opencontainers.image.revision` | ||
| - `org.opencontainers.image.licenses` |
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.
It might be helpful to better communicate better how those values are acquired?
- While it's useful to know what defaults are being provided by the action, it's also helpful to know what to expect those values are without having to experiment.
- I have opened an issue requesting better documenting the defaults as sometimes it's preferable to configure explicitly for visibility.
EDIT: I moved the bulk of this comment to the associated issue for documenting defaults.
| * [`type=sha`](#typesha) | ||
| * [`annotations` and `labels` inputs](#annotations-and-labels-inputs) | ||
| * [Default labels and annotations](#default-labels-and-annotations) | ||
| * [Customize labels and annotations](#customize-labels-and-annotations) |
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 only customization is opt-out of defaults (which could be covered in the defaults section), at which point you're back to overwriting the defaults (also valid for explaining opt-out) or adding your own (which implicitly overwrites defaults).
Additionally, there's no need to repeat labels and annotations for the subsections linked. Previously these were under a "Notes" section, so the added context was necessary.
Description
Closes #243
Currently, the
annotations:andlabels:inputs do not support the same degree of customization as other inputs likeimages:.This can have effects on output metadata, particularly for labels. The action currently sets some non-configurable labels.
metadata-action/src/meta.ts
Lines 542 to 549 in ed95091
If these labels are passed into Buildx with docker/build-push-action, they will override
LABELdirectives in the Dockerfile. It could therefore be helpful to allow users to enable or disable certain labels and annotations if they would prefer to set them withLABELdirectives instead.Changes
This PR will add customizations for the
annotations:andlabels:inputs like the ones supported for theimages:input.Global expressions will be supported.
Related