-
Notifications
You must be signed in to change notification settings - Fork 577
Add BUILDKIT_SYNTAX option handling #3157
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
Conversation
This fix allows building with a remote builder where frontend.dockerfile.v0 enabled = false in the buildkitd yaml file. Note that this change only allows the usage of BUILDKIT_SYNTAX with a custom frontend image, and using the #syntax directive in this case will still fail. Resolves: docker#3077 Signed-off-by: Will Nonnemaker <[email protected]>
This is my first pull request on this project, so any tips/guidance is much appreciated. I didn't write any unit tests for this feature, so if you want me to do that, let me know. The only way I tested this feature is by reproducing and fixing the bug locally, which I understand isn't best practice. While looking into this, I noticed a related bug where using the #syntax directive in a dockerfile will not be properly recognized if the BuildKit instance has |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at the forwardGateway
in https://github.com/moby/buildkit/blob/master/frontend/dockerfile/builder/build.go#L55-L56 . There is another parameter cmdline
as well that allows passing extra args to the external frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
on it chief |
@tonistiigi IIUC I don't think the What @wnonnemaker has here seems right from my perspective. (We also have |
@marxarelli This PR is not directly related to dockerfile syntax being disabled. Currently if you set |
I'm a bit confused because @wnonnemaker said:
But maybe you don't mean the motivation of the PR but rather the implementation? That makes sense to me.
Ah, I see. Is the correct behavior to set the |
Sorry, I think I see it now. Set |
@tonistiigi I went ahead and created a new PR, so I can contribute the changes you've requested. @wnonnemaker I added your commit there so you don't lose credit for your work. |
Fixed in #3385 |
This fix allows building with a remote builder where frontend.dockerfile.v0 enabled = false in the buildkitd yaml file.
Note that this change only allows the usage of BUILDKIT_SYNTAX with a custom frontend image, and using the #syntax directive in this case will still fail.
Resolves: #3077