Refactor build package #1466
Refactor build package #1466
Conversation
errordeveloper
force-pushed
the
build-refactor
branch
2 times, most recently
from
7aa17a9 to
2c5876a
Compare
builder/node.go
Outdated
| // LoadNodes loads and returns nodes for this builder. | ||
| // TODO: this should be a method on a Node object and lazy load data for each driver. | ||
| func (b *Builder) LoadNodes(ctx context.Context, withData bool) (_ []Node, err error) { | ||
| func (b *Builder) LoadNodes(ctx context.Context, withData, onlyAvailable bool) (_ []Node, err error) { |
There was a problem hiding this comment.
Ideally, I'd like to not modify this to take another boolean.
AvailableNodes can be renamed to LoadAvailableNodes - then LoadAvailableNodes can call LoadNodes internally, and do the filtering on that directly.
There was a problem hiding this comment.
I think that makese sense, thanks 👍
There was a problem hiding this comment.
PTAL 32c9bf0
errordeveloper
force-pushed
the
build-refactor
branch
2 times, most recently
from
3a6ce19 to
030d929
Compare
errordeveloper
requested review from
jedevc
and removed request for
crazy-max
| @@ -356,7 +81,7 @@ func toRepoOnly(in string) (string, error) { | |||
| return strings.Join(out, ","), nil | |||
| } | |||
|
|
|||
| func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt Options, bopts gateway.BuildOpts, configDir string, pw progress.Writer, dl dockerLoadCallback) (solveOpt *client.SolveOpt, release func(), err error) { | |||
| func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt options.Options, bopts gateway.BuildOpts, configDir string, pw progress.Writer, dl dockerLoadCallback) (solveOpt *client.SolveOpt, release func(), err error) { | |||
There was a problem hiding this comment.
I'd like to move this function into options package, but it looks like that would be more invloved, so I'll leave it for another PR.
- add builder method - allow filtering on load to simplify invocations Signed-off-by: Ilya Dmitrichenko <[email protected]>
Signed-off-by: Ilya Dmitrichenko <[email protected]>
This code is very tightly coupled with the rest of the build package, and is not useful elsewhere, so it makes sense to keep it where it is. Separating into another package would require more significant changes to how `driverPair` struct is used in various functions. Signed-off-by: Ilya Dmitrichenko <[email protected]>
errordeveloper
force-pushed
the
build-refactor
branch
from
030d929 to
387cc63
Compare
This adds some ideomatic earlier returns and loop continuations to avoid deeply nested coditional blocks that are several lines long. Signed-off-by: Ilya Dmitrichenko <[email protected]>
|
@jedevc the last commit should be viewed with whitespace changes ignored. I am not intending to add any more changes to this PR. |
There was a problem hiding this comment.
Not sure how much this collides with #1296, but if it does we should get that in before moving a lot of things around.
| @@ -0,0 +1,61 @@ | |||
| package options | |||
There was a problem hiding this comment.
This should not be in a separate package. If there is some import case where only types are needed then it should be types pkg under build. But I don't see how these cases should be possible as these are just the types that build pkg users for its functions and not for anything else.
There was a problem hiding this comment.
Actually, the intention was to also move toSolveOpts and decouple it from the driver abstraction, which I'm planning to embark on next. The intention would be to make a cleaner abstractions we can continue to iterrate on, right now build package has lots of things leaky pieces. Eventully such options package could provide a more high-level SovleOpts replacement.
There was a problem hiding this comment.
In addition to #1483 (comment)
I'm quite skeptical of the toSolveOpt abstraction that wouldn't just be a gimmick that only sets some of the properties, leaving the complicated ones to build pkg internals. There are lots of places where build invocations directly interact with driver interface (booting, caps detection, artifact loading, result chaining, multi-node result joining etc.). What we don't want is that there is an extra abstraction that Buildx itself doesn't really use and now when we need to implement some important user-facing features like #1377 or #1104 (that I think both change quite a lot how build pkg is called), we find out that this extra abstraction does not work in this cases and does not let us implement these features.
Assuming the abstraction that doesn't have the described maintenance issues exists and it is worth investing in writing one, it should happen within the build pkg. The problem isn't with splitting bigger files into smaller ones or changing function signatures where it makes sense, but introducing new pkg boundaries where the pkg is not usable on its own. If after the build pkg refactoring, we end up in the state where public functions(files) can be split in a way they become useful independently and without sacrificing future dev cost, then it is cheap to do the pkg split.
That PR only touches 3 lines in |
|
With the controller implementation I think we can close this one? |
#1430 moved code was moved from
build/build.go, however it is still a very long file with deeply nested logic. This change attemts to make a few improvements.