-
Notifications
You must be signed in to change notification settings - Fork 503
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
bake: fix print output #857
Conversation
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
@@ -67,29 +67,53 @@ func ReadLocalFiles(names []string) ([]File, error) { | |||
return out, nil | |||
} | |||
|
|||
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
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 --print
so output cannot be reused.cc @eitsupi
Signed-off-by: CrazyMax [email protected]