Skip to content
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

Update BuildKit dependency to use upstream Wait #5808

Merged
merged 3 commits into from
Sep 21, 2023

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Sep 21, 2023

This allow us to replace our custom logic for waiting for buildkit to be ready with the logic implemented upstream in moby/buildkit#4200.

Additionally, we need a minor fix to ensure that Directory.Without continues working, since due to a bug in buildkit, allow not found was effectively always enabled (see moby/buildkit#4051).

We also need to import some additional logic from buildkit now that the sources logic is reasonably significantly moved around, this hopefully enables the removal of the encode id hack as a follow-up.

Other similar Without methods, such as `container.WithoutMount` do not
fail if the target is not present.

However, previously in BuildKit, the llb.WithAllowNotFound was never
explicitly required. Updating to the newer BuildKit correctly respects
this option, so we need to include llb.WithAllowNotFound directly.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Copy link
Member

@TomChv TomChv left a comment

Choose a reason for hiding this comment

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

Change looks good, I validate the without directory change.

I let @vito or @sipsma double check for the id and wait changes.

I left small questions but overall this look good!

return workerInfo, nil
}
time.Sleep(retryPeriod)
ctx, cancel := context.WithTimeout(ctx, 10*time.Minute)
Copy link
Member

Choose a reason for hiding this comment

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

I let @vito or @sipsma double check but according to the CI, it seems like it perfectly work!

return []string{srctypes.HTTPSScheme}
}

func (hs *httpSource) Identifier(scheme, ref string, attrs map[string]string, platform *pb.Platform) (source.Identifier, error) {
Copy link
Member

Choose a reason for hiding this comment

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

platform isn't used in the function, should you use it as parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to take it as a parameter (it's part of the upstream interface). However, HTTP sources shouldn't change their outputs depending on platform - unlike image sources, see:

https://github.com/moby/buildkit/blob/bbe48e778f9df07eabc7fc05023c8e97e3c5c5ce/source/containerimage/source.go#L76-L82

Copy link
Contributor

@vito vito left a comment

Choose a reason for hiding this comment

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

🎉 - looks great!

@jedevc jedevc merged commit 93e4932 into dagger:main Sep 21, 2023
39 checks passed
@jedevc jedevc deleted the buildkit-update branch September 21, 2023 15:11
@gerhard gerhard added this to the v0.8.8 milestone Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants