-
Notifications
You must be signed in to change notification settings - Fork 617
bake: local dockerfile support for remote definition #2015
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1044,6 +1044,9 @@ func toBuildOpt(t *Target, inp *Input) (*build.Options, error) { | |
| if t.Dockerfile != nil { | ||
| dockerfilePath = *t.Dockerfile | ||
| } | ||
| if !strings.HasPrefix(dockerfilePath, "cwd://") { | ||
| dockerfilePath = path.Clean(dockerfilePath) | ||
| } | ||
|
|
||
| bi := build.Inputs{ | ||
| ContextPath: contextPath, | ||
|
|
@@ -1054,6 +1057,38 @@ func toBuildOpt(t *Target, inp *Input) (*build.Options, error) { | |
| bi.DockerfileInline = *t.DockerfileInline | ||
| } | ||
| updateContext(&bi, inp) | ||
| if strings.HasPrefix(bi.DockerfilePath, "cwd://") { | ||
| // If Dockerfile is local for a remote invocation, we first check if | ||
| // it's not outside the working directory and then resolve it to an | ||
| // absolute path. | ||
| bi.DockerfilePath = path.Clean(strings.TrimPrefix(bi.DockerfilePath, "cwd://")) | ||
| if err := checkPath(bi.DockerfilePath); err != nil { | ||
| return nil, err | ||
| } | ||
| var err error | ||
| bi.DockerfilePath, err = filepath.Abs(bi.DockerfilePath) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to return the absolute representation of the dockerfile path after checking its a valid one like we do for context so we can set the right dockerfile dir if context is a remote URL during build. I was also thinking reading the file in |
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else if !build.IsRemoteURL(bi.DockerfilePath) && strings.HasPrefix(bi.ContextPath, "cwd://") && (inp != nil && build.IsRemoteURL(inp.URL)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand this condition:
If Dockerfile is not remote and context is based on local dir then why would that be invalid config? Maybe a comment would help. |
||
| // We don't currently support reading a remote Dockerfile with a local | ||
| // context when doing a remote invocation because we automatically | ||
| // derive the dockerfile from the context atm: | ||
| // | ||
| // target "default" { | ||
| // context = BAKE_CMD_CONTEXT | ||
| // dockerfile = "Dockerfile.app" | ||
| // } | ||
| // | ||
| // > docker buildx bake https://github.com/foo/bar.git | ||
| // failed to solve: failed to read dockerfile: open /var/lib/docker/tmp/buildkit-mount3004544897/Dockerfile.app: no such file or directory | ||
| // | ||
| // To avoid mistakenly reading a local Dockerfile, we check if the | ||
| // Dockerfile exists locally and if so, we error out. | ||
| if _, err := os.Stat(filepath.Join(path.Clean(strings.TrimPrefix(bi.ContextPath, "cwd://")), bi.DockerfilePath)); err == nil { | ||
| return nil, errors.Errorf("reading a dockerfile for a remote build invocation is currently not supported") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this for security?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, just not supported atm, see #2015 (comment)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment to better explain the issue. |
||
| } | ||
| } | ||
| if strings.HasPrefix(bi.ContextPath, "cwd://") { | ||
| bi.ContextPath = path.Clean(strings.TrimPrefix(bi.ContextPath, "cwd://")) | ||
| } | ||
|
|
||
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.
follow-up: this should work with remote URLs as well
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.
you mean similar to what has been said in #2015 (comment) so we support reading a remote Dockerfile with a local context?