Skip to content

fix: should ignore nil client #686

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.

Sign up for GitHub

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

Merged
crazy-max merged 1 commit into from
Jul 23, 2021
Merged

fix: should ignore nil client #686

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions build/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,10 @@ func resolveDrivers(ctx context.Context, drivers []DriverInfo, auth Auth, opt ma

eg, ctx := errgroup.WithContext(ctx)
for i, c := range clients {
if c == nil {
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jul 23, 2021

Choose a reason for hiding this comment

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

Fix looks ok to address the issue, but perhaps we should also look into why this slice contains nil values in the first place, @crazy-max ?

Copy link
Copy Markdown
Member

@crazy-max crazy-max Jul 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Multi-node build support and the underlying clients iteration was first introduced while resolving drivers (#37). Client nil check was already there

buildx/build/build.go

Lines 283 to 285 in 7d312ea

if c == nil {
continue
}

but was left out to be included in #535.

but perhaps we should also look into why this slice contains nil values in the first place,

Yes agree, might be an issue with the context dialer while creating the buildkit client for docker-container driver. Will take a look.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jul 23, 2021

Choose a reason for hiding this comment

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

Was initially thinking, perhaps the slice is initialized with a length/size (not taking into account that filtering might happen). Haven't looked at the code though (was reading from my phone), but though it wouldn't hurt to leave a comment ☺️🤗

Copy link
Copy Markdown
Member

@crazy-max crazy-max Jul 23, 2021

Choose a reason for hiding this comment

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

Yes you did well :)

continue
}

func(i int, c *client.Client) {
eg.Go(func() error {
clients[i].Build(ctx, client.SolveOpt{}, "buildx", func(ctx context.Context, c gateway.Client) (*gateway.Result, error) {
Expand Down