Skip to content

Conversation

@jedevc
Copy link
Collaborator

@jedevc jedevc commented May 23, 2023

This was missing, since the driver property can only be fully populated after loading nodes from disk. So we add logic to load the nodes, and check for an error, which ensures that the "docker" driver is always correctly present in the progress description.

I think this was introduced in #1737, when I refactored the progress printing.

Before:

$ buildx build . --builder=desktop-linux
[+] Building 2.2s (10/10) FINISHED                                                                                           :desktop-linux

After:

$ buildx build . --builder=desktop-linux
[+] Building 0.9s (6/6) FINISHED                                                                                       docker:desktop-linux

This was missing, since the driver property can only be fully populated
after loading nodes from disk. So we add logic to load the nodes, and
check for an error, which ensures that the "docker" driver is always
correctly present in the progress description.

Signed-off-by: Justin Chadwell <[email protected]>
@jedevc jedevc added the kind/bug Something isn't working label May 23, 2023
@jedevc jedevc requested a review from crazy-max May 23, 2023 09:41
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Looking at

// TODO: this should not be loaded this side of the controller api
b, err := builder.New(dockerCli,
builder.WithName(in.Builder),
builder.WithContextPathHash(contextPathHash),
)
if err != nil {
return nil, nil, err
}
if err = updateLastActivity(dockerCli, b.NodeGroup); err != nil {
return nil, nil, errors.Wrapf(err, "failed to update builder last activity time")
}
nodes, err := b.LoadNodes(ctx, false)
if err != nil {
return nil, nil, err
}
it seems we instantiate the builder and load nodes twice now. Might look at this in follow-up.

@jedevc jedevc merged commit 341fb65 into docker:master May 25, 2023
@jedevc jedevc deleted the fix-missing-driver-in-build branch May 25, 2023 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants