-
Notifications
You must be signed in to change notification settings - Fork 545
bake: allow annotations to be set on the command line #2997
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
Conversation
@@ -558,7 +558,7 @@ func (c Config) newOverrides(v []string) (map[string]map[string]Override, error) | |||
// IMPORTANT: if you add more fields here, do not forget to update | |||
// docs/bake-reference.md and https://docs.docker.com/build/bake/overrides/ | |||
switch keys[1] { | |||
case "output", "cache-to", "cache-from", "tags", "platform", "secrets", "ssh", "attest", "entitlements", "network": | |||
case "output", "cache-to", "cache-from", "tags", "platform", "secrets", "ssh", "attest", "entitlements", "network", "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.
Should we do the same for labels
?
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.
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.
For what it's worth, I don't think labels has the same issue. It uses Value
from the overrides:
Lines 891 to 898 in 52f503e
case "labels": | |
if len(keys) != 2 { | |
return errors.Errorf("invalid format for labels, expecting labels.<name>=<value>") | |
} | |
if t.Labels == nil { | |
t.Labels = map[string]*string{} | |
} | |
t.Labels[keys[1]] = &value |
Compare to annotations
:
Lines 973 to 974 in 52f503e
case "annotations": | |
t.Annotations = append(t.Annotations, o.ArrValue...) |
So this might already work for labels
? It looks like it's documented in there to work.
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.
Just to clarify, this switch
is a list of "use ArrValue
to fill in the overrides" and the default is to use Value
if you're not in the list.
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.
Ah yes indeed, thanks
Annotations were not merged correctly. The overrides in `ArrValue` would be merged, but the section of code setting them from the command line did not include `annotations` in the list of available attributes so the command line option was completely discarded. Signed-off-by: Jonathan A. Sternberg <[email protected]>
29f30b8
to
d6fdf83
Compare
Annotations were not merged correctly. The overrides in
ArrValue
would be merged, but the section of code setting them from the command line did not includeannotations
in the list of available attributes so the command line option was completely discarded.Fixes #2989.