-
Notifications
You must be signed in to change notification settings - Fork 481
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
Build: Support for printing outline/targets of the current build #1100
Conversation
Demo of this feature in action: https://drive.google.com/file/d/1H92gzwctwRkllVKtgF6lDpDmk-4NACLs/view |
Nice! |
That's cool. We used to have Dockerfiles like this, building in bake:
but Dependabot can't then update them. It was annoying because we want to include the PHP version in the tag. I guess now we can inline the version into the Dockerfile again, run this outline to extract the version(s), then use those versions in the tags we push? It would be very interesting in future to be able to access outline values from bake itself |
This is somewhat tricky as it can easily go to an infinite loop like this. Your bake config modifies the Dockerfile args and then Dockerfile args modify bake config again. |
As
For a follow-up, maybe also be able to specify an output format that defaults to tabwriter if not specified:
About the output I think we should hide the progress output and just display solve errors:
|
60c543a
to
aca95ba
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.
Left some initial thoughts 🎉
Not sure about having a fallback like specified, silently upgrading to a new frontend seems like we could cause subtle compatability issues - if we do go with a fallback, I think it's worth warning the user that we've done so. Also would need a way to pick a fallback for different frontends, we shouldn't just pick dockerfile if we've specified a different syntax.
For a follow-up, maybe also be able to specify an output format that defaults to tabwriter if not specified:
👍 I think this is nice, would make this info very readable 🎉
About the output I think we should hide the progress output and just display solve errors:
I don't think we should do anything special here, as I understand the code, a frontend could possibly choose to continue it's normal operation even with a subrequest (and do some supplement on top). I think we should support use case though, maybe with a new --progress=none
option or something in a buildkit follow-up (could be useful for other scenarios as well).
for { | ||
if opt.PrintFunc != nil { | ||
if _, ok := req.FrontendOpt["frontend.caps"]; !ok { | ||
req.FrontendOpt["frontend.caps"] = "moby.buildkit.frontend.subrequests+forward" |
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.
Think it would be nice to have consts for the subrequest caps on the buildkit side for these caps similar to solver/pb/caps.go
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 should be defined in some package that defines keys for Dockerfile frontend compatibility interface. https://github.com/moby/buildkit/blob/master/frontend/dockerfile/builder/build.go#L43-L47 Unfortunately no such package exists atm
resultHandleFunc(dp.driverIndex, &ResultContext{cc, res}) | ||
var isFallback bool | ||
var origErr error | ||
for { |
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.
Not sure if we should use a loop here to do the retries? As I understand it, it's so that we can have a second try with a fallback, but that might be easier to read in-serial, instead of with the loop/continue which is hard to reason about control flow.
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.
Not sure I understand. Without the loop, a bunch of this code would need to be duplicated.
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 was thinking we could try and move the logic into a separate function/method, as a generic makeRequest
or something function, then we could use a bool return to check if it was successful, and if not, we could make a fallback call to the same function, which would keep the logic needed in BuildWithResultHandler
to a minimum.
rr.ExporterResponse = map[string]string{} | ||
} | ||
for k, v := range printRes { | ||
rr.ExporterResponse[k] = string(v) |
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.
Because we store the values in here, are they going to be output to the metadata file as well? I think that makes sense.
I wonder if we need to do something here to make sure we don't write to any known keys here, if the frontend's returned Metadata contains a duplicate key for whatever reason, we might potentially corrupt some data here. Maybe it's worth keeping this info separate with a composite struct?
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.
Actually, I wonder if we could do something similar to annotations, where we have the prefix annotation.
, here we could have the prefix subrequest.
or something.
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.
Because we store the values in here, are they going to be output to the metadata file as well?
Yes.
frontend's returned Metadata contains a duplicate key for whatever reason,
How can it be duplicated if it is a map?
here we could have the prefix subrequest. or something.
I don't think that would be right. The point of subrequests is to take a different code path and return results for it, so it is not "do build and also return extra keys that would need to be namespace". Oth, subrequest build does not mean that it needs to return json/text. A subrequest could for example do a build with a different configuration and return a different image. In that case, it does want to return the correct image keys so they get exported correctly, not namespaced special keys.
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.
Sorry, I meant duplicated values between res.Metadata
and rr.ExporterResponse
since we do a merge here.
Agreed though, thanks for the clarification on subrequests.
for _, field := range fields { | ||
parts := strings.SplitN(field, "=", 2) | ||
if len(parts) == 2 { | ||
if parts[0] == "format" { |
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 feels a bit confusing to me since we already have the --format
flag - I guess this one is to indicate the source format, as opposed to the output format? I think it might be good to have a different name for this to help clarify, or maybe is there a way we could avoid needing to have to specify the format here?
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 was made to match #1100 (comment)
|
||
func printValue(printer printFunc, version string, format string, res map[string]string) error { | ||
if format == "json" { | ||
fmt.Fprintln(os.Stdout, res["result.json"]) |
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 we should have result.{txt,json}
as consts in buildkit maybe, or even a func (format string) (filename string)
somewhere, to avoid having this info repeated between client-server.
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 is not hardcoded anywhere but defined in the subrequest definition that frontend shows. If handling a specific one, client should look what outputs it has from the description. This is a generic "best-effort" because we let the user specify any function name/
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.
sources
does not seem decoded: https://gist.github.com/crazy-max/dfb1e5fc84f258be128e2fba90c467e1
Could you use the decodeExporterResponse
?:
Lines 607 to 623 in 98f9f80
func decodeExporterResponse(exporterResponse map[string]string) map[string]interface{} { | |
out := make(map[string]interface{}) | |
for k, v := range exporterResponse { | |
dt, err := base64.StdEncoding.DecodeString(v) | |
if err != nil { | |
out[k] = v | |
continue | |
} | |
var raw map[string]interface{} | |
if err = json.Unmarshal(dt, &raw); err != nil || len(raw) == 0 { | |
out[k] = v | |
continue | |
} | |
out[k] = json.RawMessage(dt) | |
} | |
return out | |
} |
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.
A source is the original source file that we don't control. It can be any kind of bytes. I think we shouldn't make assumptions about this. Even if it is a Dockerfile, showing some encoded string in here for the whole big file would not make it clearer and the user would need to do some decoding from a string to try to get the original form.
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 is not encoded JSON so decodeExporterResponse
would not do anything here.
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 right that makes sense.
aca95ba
to
bea1214
Compare
Print flag can be used to make additional information requests about the build and print their results. Currently Dockerfile supports: outline, targets, subrequests.describe Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
bea1214
to
c834ba1
Compare
I noticed the exporter still runs with this. |
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.
One minor thing mentioned already, not sure if anyone cares.
Otherwise LGTM
Might be nice to work out a new subcommand for these requests like |
This is intentional as we will not make assumptions about what functions frontends will implement. These functions can run the regular(or custom) build and return results. These results would be exported with |
This implements
docker buildx build --print=[value]
for running a secondary request that frontend supports and print its value. This is buildx side of moby/buildkit#2841 support.Currently accepted values are:
To test, check out the PR and run
make install
. Buildkit update is not needed. If you don't use the updated frontend with outline support there is a fallback to load one in this PR.Looking for feedback on the flag names, output formatting and if more data would be needed in moby/buildkit#2841 . We can't really use flags like
--list
or--describe
as we want this subrequest logic to be extendable in the future with more use cases. Any frontend can define that it implements a certain function and then that function can be called with--print
atm.Related discussion on bake level #1072