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

bake: fix BAKE_CMD_CONTEXT relative path resolution #1840

Merged
merged 8 commits into from Jun 6, 2023

Conversation

jedevc
Copy link
Collaborator

@jedevc jedevc commented May 25, 2023

🛠️ Fixes path resolution issue with BAKE_CMD_CONTEXT.

In https://docs.docker.com/build/bake/remote-definition/, the following snippet no longer works:

$ docker buildx bake "https://github.com/tonistiigi/buildx.git#remote-test" --print
{
  "target": {
    "default": {
      "context": ".",
      "dockerfile": "Dockerfile",
      "dockerfile-inline": "FROM alpine\nWORKDIR /src\nCOPY . .\nRUN ls -l \u0026\u0026 stop\n"
    }
  }
}

Instead returning the following error:

$ docker buildx bake "https://github.com/tonistiigi/buildx.git#remote-test" --print
ERROR: Rel: can't make . relative to /home/jedevc/Documents/Docker/buildx

This issue comes from

buildx/bake/bake.go

Lines 1007 to 1010 in 341fb65

rel, err := filepath.Rel(wd, p)
if err != nil {
return err
}

To resolve this issue, we need to ensure that the passed path is an absolute path, instead of the relative path ..

To prevent further regression, I add a couple of tests to build against a remote git repo (which is where most of the code in this PR comes from 😱).

cc @crazy-max @dvdksn

@jedevc jedevc force-pushed the fix-check-path-for-bake-cmd-context branch from c7647f4 to 41c613d Compare May 25, 2023 14:31
@jedevc jedevc added area/bake kind/bug Something isn't working labels May 25, 2023
@jedevc jedevc force-pushed the fix-check-path-for-bake-cmd-context branch 2 times, most recently from 5296eaa to cf100e3 Compare May 25, 2023 16:55
@jedevc jedevc added this to the v0.11.0 milestone May 31, 2023
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Can we have a test case for

buildx/bake/bake.go

Lines 1011 to 1013 in 7cef021

if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
return errors.Errorf("path %s is outside of the working directory, please set BAKE_ALLOW_REMOTE_FS_ACCESS=1", p)
}
?

@jedevc
Copy link
Collaborator Author

jedevc commented Jun 6, 2023

Can we have a test case

@crazy-max sure, will work on this.

@jedevc jedevc force-pushed the fix-check-path-for-bake-cmd-context branch from efe200a to 537a643 Compare June 6, 2023 15:03
Signed-off-by: Justin Chadwell <me@jedevc.com>
- Adds a new GitServeHTTP function to start an http server to serve a
  target git repository.
- Adds a new GitDir helper method to get the path to the .git
  directory
- Updates the GitAdd method to take a variable number of files

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Fixed in 12b6a3a, but now we have
regression tests! So we can add a check that we don't break this
behavior again.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the fix-check-path-for-bake-cmd-context branch from 537a643 to d34103b Compare June 6, 2023 15:19
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Iiuc the util/gitutil serves dual purposes atm and is used for reading VCS info for builds as well as test utilities. This isn't ideal and these responsibilities should be split. Not needed in this PR though.

@jedevc jedevc requested a review from crazy-max June 6, 2023 15:35
@jedevc jedevc merged commit 696770d into docker:master Jun 6, 2023
58 checks passed
@jedevc jedevc deleted the fix-check-path-for-bake-cmd-context branch June 6, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bake kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants