-
Notifications
You must be signed in to change notification settings - Fork 616
bake: fix print output #857
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
tonistiigi
left a comment
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.
PTAL Examples in https://github.com/docker/buildx/blob/master/docs/reference/buildx_bake.md
Can we have a test for this?
9ebe9fd to
be0620f
Compare
|
Don't you also need a test case for a non "default" group name? |
64fa5fc to
6ea4b44
Compare
bake/bake.go
Outdated
| } | ||
|
|
||
| func ReadTargets(ctx context.Context, files []File, targets, overrides []string, defaults map[string]string) (map[string]*Target, []*Group, error) { | ||
| func ReadTargets(ctx context.Context, files []File, inp *Input, targets []string, overrides []string, defaults map[string]string) (map[string]*Target, map[string]*Group, map[string]build.Options, error) { |
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.
What's the reason for changing this signature? Can't you just add the group setting and leave the rest of the function as is.
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.
Thought TargetsToBuildOpt had some impact on groups context but it's actually only against targets.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Generally, I don't mind the map return but as this is going to be picked to release branch probably better to keep the change minimal. We can follow up with the type change in another PR that will not be picked later.
f670c3c to
ed08b82
Compare
tonistiigi
left a comment
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.
This doesn't seem to handle
target "default" {}
case
ec3f9da to
fb9381c
Compare
|
Signed-off-by: CrazyMax <[email protected]>
fixes #856
targets groups are not correctly rendered while being printed with
bake --printso output cannot be reused.cc @eitsupi
Signed-off-by: CrazyMax [email protected]