Planned change to git clone logic

I am planning to change how we clone GitHub pull requests in a future version of drone. The below script demonstrates what this new logic will look like:

#!/bin/sh
set -e
set -x

git clone -b master --single-branch https://github.com/octocat/Spoon-Knife.git \
  /drone/src/github.com/octocat/Spoon-Knife

cd Spoon-Knife
git fetch origin pull/14596/head:
git rebase 26923a8f37933ccc23943de0d4ebd53908268582

Benefits include:

  • Improved error when merge conflicts exist
  • Uses the correct sha for pull request synchronizations
  • Does not rely on the merge ref
  • Rebase preserves the head sha, which is important for coveralls, codecov, etc

There are of course many ways to clone a project and checkout a commit. This will be the default, but you will always have the ability to override this default behavior with a plugin.

7 Likes

In this example, it seems like master is the target branch of the pull request and 26923a8f37933ccc23943de0d4ebd53908268582 is the commit being tested. Is my understanding correct?

@zjs yes it is

You can see the commit and pull request here if you want to test and / or improve upon the script.

I’d like to implement something similar in a custom clone plugin. Is there a way to get the target branch of a pull request?

for pull requests, the DRONE_BRANCH will always be set to the target branch. Drone does not, at this time, track the source branch.

The step I came up as a replacement of the default clone logic is:

clone:
  clone:
    image: plugins/git
    commands:
      - git clone -b "$DRONE_BRANCH" "$DRONE_REMOTE_URL" .
      - git fetch origin "$DRONE_COMMIT_REF"
      - if [ "$DRONE_BUILD_EVENT" = "pull_request" ]; then git merge "$DRONE_COMMIT_SHA"; else git checkout -qf "$DRONE_COMMIT_SHA"; fi
      - git submodule update --init --recursive

Note that I have DRONE_GITHUB_MERGE_REF=false.

If someone see something obviously wrong or that could be improved, I would be happy to here feedback =)

@allanrenucci How’s this approach working out for you? Any weird gotchas you’ve noticed?

@bradrydzewski Thanks for taking the time to outline this. I’m wary of “rolling my own” approach for something so critical. Just wondering if you’d consider this to be a feature that’s still far-out in terms of landing in a release? Or something we might see in the coming weeks/months?

@geekdave I had issues with git clone (taking forever) and git merge (failing, asking for git username and email) in the approach I described above. Our current solution that seems to be working so far is:

clone:
  git:
    image: plugins/git
    # We clone submodules ourselves in ci-clone
    recursive: false

pipeline:
  # We add a custom clone step to workaround a bug with GitHub merge ref
  clone:
    image: plugins/git
    commands:
      - ./ci-clone

And ci-clone defined as:

#!/usr/bin/env sh
set -eux

# if build is PR rebase on top of target branch
if [ "$DRONE_BUILD_EVENT" = "pull_request" ]; then
  git config user.email "[email protected]"
  git config user.name "Some Name"
  git pull  "$DRONE_REMOTE_URL" "$DRONE_BRANCH"
fi

git submodule update --init --recursive

I had issues with git clone (taking forever) and git merge (failing, asking for git username and email) in the approach I described above.

This could indicate you are not generating the netrc file which is used for automatic authentication. See https://github.com/drone-plugins/drone-git/blob/master/utils.go#L44

Also looking at your example, it would only work with intra-repository pull requests, since you would need to use the merge ref if the pull request is coming from a fork.

Should master be rebasing on the branch’s commit?

I was wondering if that could create unexpected issues / not the same result when the branch is actually merged into master later.

I started building a proof of concept of the new pull request logic here: https://github.com/drone-plugins/drone-git/tree/next

The implementation has some important properties:

  • uses the head ref for pull requests and manually performs the merge
  • guarantees the correct pull request sha and will fail when encountering merge conflicts
  • preserves the branch and sha and avoids a detached HEAD state (supporting tools that use git rev-parse HEAD)

There are some limitations with the implementation

  • pull requests tested with github only; ref logic in place for gitlab, gitea, gogs and bitbucket
  • does not yet work with github deployment events
  • does not yet support skip verify
  • does not yet support non-empty directories (important if caching .git directory)
  • does not support submodules (this is not planned, cloning submodules should instead be handled as a step in your pipeline)

There is solid unit testing in place and I think this provides a good base that we can build upon. If this is something you would like to help accelerate, please consider donating a few hours of time to help test and provide feedback and ways to improve.

You can use this plugin today for non-critical projects with the following syntax:

clone:
  git:
    image: plugins/git:next

pipeline: ...
2 Likes

Glad to be a guinea pig, @bradrydzewski! I fired up this new image in my clone phase, and got this error right out-of-the gate on the /pr job (/push job ran normally):

chmod: 600: No such file or directory
Initialized empty Git repository in /drone/src/github.com/myorg/dummy-repo/.git/
+ git fetch origin +refs/heads/master:
From https://github.com/myorg/dummy-repo
 * branch            master     -> FETCH_HEAD
 * [new branch]      master     -> origin/master
+ git checkout master
Branch master set up to track remote branch master from origin.
Already on 'master'
+ git fetch origin pull/28/head:
From https://github.com/myorg/dummy-repo
 * branch            refs/pull/28/head -> FETCH_HEAD
+ git rebase 38482f303c72888a96d50338a5d8b466db9c097d
First, rewinding head to replay your work on top of it...

*** Please tell me who you are.

Run

  git config --global user.email "[email protected]"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: unable to auto-detect email address (got '[email protected](none)')

Here’s my drone.yml:

clone:
  git:
    image: plugins/git:next
    
pipeline:
  canary:
    image: alpine
    commands:
      - echo "This is a test to make sure Drone is running jobs."

I added a default email and name (via environment variable) which should satisfy that github prompt if it appears again. A new image is now available. Note that I didn’t have a test PR to reproduce the prompt, so this is untested, but according to git docs it should work.

export GIT_AUTHOR_NAME=${CI_COMMIT_AUTHOR_NAME=drone}
export GIT_AUTHOR_EMAIL=${[email protected]}
export GIT_COMMITTER_NAME=${GIT_AUTHOR_NAME}
export GIT_COMMITTER_EMAIL=${GIT_AUTHOR_EMAIL}

Thanks - I’ll try this. Since I already have the :next version installed on several agent machines, is there an easy way to force the upgrade without manually removing that image from each machine?

is there an easy way to force the upgrade

yes, with the following attribute:

clone:
  git:
    image: plugins/git:next
+   pull: true

First of all, big thanks for this. Outdated merge heads from Github started to pop up more and more in our deployment.

We now started using this in a few repo. First executions look good, and we are happy that this is hash based now.

There was one case though that may be of interest: in one of our long running branches the rebase failed with a conflict, while a manual merge of master to this branch succeeded without conflict. Retrying the build after this merge, the rebase succeeded as nothing left to rebase really.

I’m thinking that git rebase <hash> is perhaps more fragile than git reset --hard <hash> followed by a git merge master. But I’m sure you have reasons why it’s a rebase and not a merge.

Thanks for your work!

1 Like

not really, it was just the first thing that passed unit tests :slight_smile:

The primary goal was was to preserve the hash and not create a new merge commit or create a detached head state. If the unit tests pass without modification with reset --hard I am open to adjusting. Thanks for testing, and for the feedback!

Just wanted to verify that we’ll be able to continue to pull the specific branch/commit without doing any of the auto-rebasing stuff. In our case, the extra magic hurts us more than it helps.

@gtaylor I recommend either testing the shell script locally, or maybe even testing with one of your repositories if possible. If we identify issues over the next few weeks, it will give us time to fix before this becomes the default in 0.9

Link to script https://github.com/drone-plugins/drone-git/blob/next/posix/clone-pull-request