Refactor builder and drivers info logic#1430
Conversation
crazy-max
force-pushed
the
builder-pkg
branch
from
8930e23 to
d3a0a07
Compare
jedevc
left a comment
jedevc
left a comment
There was a problem hiding this comment.
I like the refactor 🎉 - left a couple comments.
I'm curious, I can't quite see where the performance improvement has come from 👀
This comment was marked as resolved.
This comment was marked as resolved.
crazy-max
force-pushed
the
builder-pkg
branch
from
d3a0a07 to
94e1113
Compare
crazy-max
changed the title
crazy-max
force-pushed
the
builder-pkg
branch
3 times, most recently
from
e4df400 to
1062c23
Compare
crazy-max
force-pushed
the
builder-pkg
branch
from
1062c23 to
24b093a
Compare
jedevc
left a comment
jedevc
left a comment
There was a problem hiding this comment.
LGTM, just a minor function rename :)
build/build.go
Outdated
|
|
||
| func filterAvailableDrivers(drivers []DriverInfo) ([]DriverInfo, error) { | ||
| out := make([]DriverInfo, 0, len(drivers)) | ||
| func filterAvailableDrivers(nodes []builder.Node) ([]builder.Node, error) { |
There was a problem hiding this comment.
| func filterAvailableDrivers(nodes []builder.Node) ([]builder.Node, error) { | |
| func filterAvailableNodes(nodes []builder.Node) ([]builder.Node, error) { |
Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
crazy-max
force-pushed
the
builder-pkg
branch
from
24b093a to
cc01caa
Compare
| func(i int, n store.Node) { | ||
| eg.Go(func() error { |
There was a problem hiding this comment.
The code would be easier to read if at least one of these annonymous function was tunred into a mehtod.
There was a problem hiding this comment.
Yes agree, I put a comment on this method so we can track this as follow-up: https://github.com/docker/buildx/pull/1430/files#diff-7c6ac5bb2a9c2322f716d7b66113042e64298fbdc5cba600abed692a0c2a6570R38
errordeveloper
left a comment
errordeveloper
left a comment
There was a problem hiding this comment.
Thanks a lot, this is a significant improvement! I might follow up with a few more things, as I'm trying to use some of the same functions in another project.
| if b.err == nil && err != nil { | ||
| b.err = err | ||
| } | ||
| _, _ = b.LoadNodes(timeoutCtx, true) |
There was a problem hiding this comment.
Why is this ignoring the error instead of setting it to the builder?
4 participants
Moves builder and drivers info logic from
commandspkg to a dedicated client pkg so we can easily interact with builders and drivers. Refactoring reduced latency for operations when loading drivers data or checking already validated endpoints:Added some TODOs as there is still room for improvements.