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 protocol detection #822

Merged
merged 1 commit into from Nov 2, 2021

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Nov 2, 2021

while investigating with the current state of git:// protocol (https://github.blog/2021-09-01-improving-git-protocol-security-github/) I encounter an issue with bake and the ability to use a remote definition using https:

$ docker buildx bake "https://github.com/docker/cli.git#master" --print
#1 [internal] load remote build context
#1 DONE 0.0s
error: failed to parse https://github.com/docker/cli.git#master: parsing yaml: yaml: line 99: mapping values are not allowed in this context, parsing hcl: https://github.com/docker/cli.git#master.hcl:225,21-22: Unsupported operator; Bitwise operators are not supported. Did you mean boolean AND ("&&")?, and 75 other diagnostic(s)

With this PR we now detect if a git-like url is used:

$ docker buildx bake "https://github.com/docker/cli.git#master" --print
#1 [internal] load git source https://github.com/docker/cli.git
#1 0.036 hint: Using 'master' as the name for the initial branch. This default branch name
#1 0.036 hint: is subject to change. To configure the initial branch name to use in all
#1 0.036 hint: of your new repositories, which will suppress this warning, call:
#1 0.036 hint:
#1 0.036 hint:  git config --global init.defaultBranch <name>
#1 0.036 hint:
#1 0.036 hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
#1 0.036 hint: 'development'. The just-created branch can be renamed via this command:
#1 0.036 hint:
#1 0.036 hint:  git branch -m <name>
#1 0.037 Initialized empty Git repository in /var/lib/buildkit/runc-overlayfs/snapshots/snapshots/626/fs/
#1 0.491 ref: refs/heads/master HEAD
#1 0.494 3fb4fb83dfb5db0c0753a8316f21aea54dab32c5       HEAD
#1 0.944 3fb4fb83dfb5db0c0753a8316f21aea54dab32c5       refs/heads/master
#1 0.459 ref: refs/heads/master HEAD
#1 0.462 3fb4fb83dfb5db0c0753a8316f21aea54dab32c5       HEAD
#1 2.859 From https://github.com/docker/cli
#1 2.859  * [new branch]      master     -> master
#1 2.859  * [new branch]      master     -> origin/master
#1 DONE 3.7s
{
  "group": {
    "default": [
      "binary"
    ]
  },
  "target": {
    "binary": {
      "context": "https://github.com/docker/cli.git",
      "dockerfile": "Dockerfile",
      "args": {
        "BASE_VARIANT": "alpine",
        "COMPANY_NAME": "",
        "GO_STRIP": "",
        "VERSION": ""
      },
      "target": "binary",
      "platforms": [
        "local"
      ],
      "output": [
        "build"
      ]
    }
  }
}

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@crazy-max
Copy link
Member Author

will fix our docs relative to the deprecation of git:// in a follow-up.

@crazy-max crazy-max added this to the v0.7.0 milestone Nov 2, 2021
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.

What if we call detectGitContext before detectHTTPContext. Wouldn't that also fix this issue?

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max
Copy link
Member Author

What if we call detectGitContext before detectHTTPContext. Wouldn't that also fix this issue?

Yes and it might be better cause detectHTTPContext should still be valid even if it's a git-like uri.

@tonistiigi tonistiigi merged commit 7f0e375 into docker:master Nov 2, 2021
@crazy-max crazy-max deleted the fix-bake-git-protoc branch November 3, 2021 19:39
@crazy-max crazy-max mentioned this pull request Nov 3, 2021
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

2 participants