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

build: don't stat directories that we don't plan to upload #5768

Merged
merged 1 commit into from
May 5, 2022
Merged

Conversation

nicks
Copy link
Member

@nicks nicks commented May 4, 2022

Hello @milas, @landism,

Please review the following commits I made in branch nicks/walk:

c1a8373 (2022-05-04 19:06:56 -0400)
build: don't stat directories that we don't plan to upload
creates some temporary patches on buildkit until we can upstream them

fixes #5745

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@nicks nicks requested review from landism and milas May 4, 2022 23:07
@nicks nicks force-pushed the nicks/walk branch 2 times, most recently from 7304c29 to 157ed50 Compare May 4, 2022 23:09
@nicks
Copy link
Member Author

nicks commented May 4, 2022

I'm hoping the upstream patches will be accepted, because they're pretty minimal/non-invasive (which you can see in the vendor diff). But figure we'll have to patch anyway because we're on an older version of buildkit.

@milas
Copy link
Contributor

milas commented May 5, 2022

But figure we'll have to patch anyway because we're on an older version of buildkit.

FWIW if the fork(s) become onerous to maintain, we could probably get most of the way there by using the existing Excludes field on SyncedDir. We'd have to populate from v1alpha1.IgnoreDef, which would require some care to avoid it kicking right back down the slow path, so it's not ideal for a variety of reasons

@nicks
Copy link
Member Author

nicks commented May 5, 2022

ya, it's tricky because ExcludePatterns MUST be relative to the context, so would need to do a fair bit of re-formatting

creates some temporary patches on buildkit until we can upstream them

fixes #5745
@nicks nicks merged commit b0d07f8 into master May 5, 2022
@nicks nicks deleted the nicks/walk branch May 5, 2022 18:17
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.

docker_build is not respecting .dockerignore
2 participants