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.

Sign up for GitHub

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

scripts: add pull_github_pr.sh #2484

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

scripts: add pull_github_pr.sh #2484

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions scripts/pull_github_pr.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
#!/bin/bash

# Script for pulling a github pull request
# along with generating a merge commit message.
# Example usage for pull request #6007:
#
# git checkout master
# git pull
# ./scripts/pull_github_pr.sh 6007

set -e

gh_hosts=~/.config/gh/hosts.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, is this ~/.config/gh/hosts.yml something standard? Where is it documented?

Copy link
Member Author

Choose a reason for hiding this comment

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

It comes from the gh application, that I only used in order to establish login credentials once.

jenkins_url="https://jenkins.scylladb.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

this variable is not used.

FORCE=$2
Copy link
Contributor

Choose a reason for hiding this comment

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

this variable is not used.

ORANGE='\033[0;33m'
Copy link
Contributor

Choose a reason for hiding this comment

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

this variable is not used.

NC='\033[0m'
Copy link
Contributor

Choose a reason for hiding this comment

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

this variable is not used.


if [[ ( -z "$GITHUB_LOGIN" || -z "$GITHUB_TOKEN" ) && -f "$gh_hosts" ]]; then
GITHUB_LOGIN=$(awk '/user:/ { print $2 }' "$gh_hosts")
GITHUB_TOKEN=$(awk '/oauth_token:/ { print $2 }' "$gh_hosts")
fi

if [[ -z "$GITHUB_LOGIN" || -z "$GITHUB_TOKEN" ]]; then
echo 'Please set $GITHUB_LOGIN and $GITHUB_TOKEN'
exit 1
fi

if [ -z "$1" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Silly nitpick: please consider picking either the newer (circa 1983 ;-)) ksh double bracket ([[) or the old-school single bracket (/bin/[]) - but not mix them both in the same script.

echo Please provide a github pull request number
Copy link
Contributor

Choose a reason for hiding this comment

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

can we only use spaces instead of using both tabs and spaces for indent?

exit 1
fi

for required in jq curl; do
if ! type $required >& /dev/null; then
echo Please install $required first
exit 1
fi
done

curl() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider renaming this function. I find it confusing that you take a command that has an existing meaning, and redefine it. The fact that you had to use the obscure builtin "command" is a tell-tale sign it's confusing.

local opts=()
if [[ -n "$GITHUB_LOGIN" && -n "$GITHUB_TOKEN" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

GITHUB_LOGIN and GITHUB_TOKEN are always set, so we don't need to check them here. or, we can do something like:

curl() {
    if [[ ( -z "$GITHUB_LOGIN" || -z "$GITHUB_TOKEN" ) && -f "$gh_hosts" ]]; then
        GITHUB_LOGIN=$(awk '/user:/ { print $2 }' "$gh_hosts")
        GITHUB_TOKEN=$(awk '/oauth_token:/ { print $2 }' "$gh_hosts")
     fi

     if [[ -z "$GITHUB_LOGIN" || -z "$GITHUB_TOKEN" ]]; then
        echo 'Please set $GITHUB_LOGIN and $GITHUB_TOKEN'
        exit 1
    fi

    command curl --user "${GITHUB_LOGIN}:${GITHUB_TOKEN}" "$@"
}

as these two variables are "closer" to this function, and we only set them once they are set by this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that there's no need to check these variables again (we checked above that they must be set, if they aren't, the script exited). I also agree that it's nicer to just use crystal-clear shell syntax like you suggested instead of the bizarre arrays used by Avi's code.
But I wouldn't move the check for these variables into the function - I think it's better to check them up-front in the beginning of the script, like Avi did.

opts+=(--user "${GITHUB_LOGIN}:${GITHUB_TOKEN}")
fi
command curl "${opts[@]}" "$@"
}


NL=$'\n'

PR_NUM=$1
# convert full repo URL to its project/repo part, in case of failure default to origin/master:
REMOTE_SLASH_BRANCH="$(git rev-parse --abbrev-ref --symbolic-full-name @{upstream} \
Copy link
Contributor

Choose a reason for hiding this comment

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

extraneous space after name.

|| git rev-parse --abbrev-ref --symbolic-full-name master@{upstream} \
|| echo 'origin/master')"
REMOTE="${REMOTE_SLASH_BRANCH%/*}"
REMOTE_URL="$(git config --get "remote.$REMOTE.url")"
PROJECT=`sed 's/[email protected]://;s#https://github.com/##;s/\.git$//;' <<<"${REMOTE_URL}"`
PR_PREFIX=https://api.github.com/repos/$PROJECT/pulls

echo "Fetching info on PR #$PR_NUM... "
Copy link
Contributor

Choose a reason for hiding this comment

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

extraneous space after ....

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe he wanted "echo -n"?

PR_DATA=$(curl -s $PR_PREFIX/$PR_NUM)
MESSAGE=$(jq -r .message <<< $PR_DATA)
if [ "$MESSAGE" != null ]
Copy link
Contributor

Choose a reason for hiding this comment

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

to be more consistent with the rest of this script, how about moving "next" to the previous line (and add a ;)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I would do the exact opposite. Having the then on a new line, and no ";", is the more traditional and better looking, syntax.

then
# Error message, probably "Not Found".
echo "$MESSAGE"
exit 1
fi
PR_TITLE=$(jq -r .title <<< $PR_DATA)
echo " $PR_TITLE"
PR_DESCR=$(jq -r .body <<< $PR_DATA)
PR_LOGIN=$(jq -r .head.user.login <<< $PR_DATA)
echo -n "Fetching full name of author $PR_LOGIN... "
USER_NAME=$(curl -s "https://api.github.com/users/$PR_LOGIN" | jq -r .name)
echo "$USER_NAME"

git fetch "$REMOTE" pull/$PR_NUM/head

nr_commits=$(git log --pretty=oneline HEAD..FETCH_HEAD | wc -l)

closes="${NL}${NL}Closes ${PROJECT}#${PR_NUM}${NL}"

if [[ $nr_commits == 1 ]]; then
commit=$(git log --pretty=oneline HEAD..FETCH_HEAD | awk '{print $1}')
message="$(git log -1 "$commit" --format="format:%s%n%n%b")"
if ! git cherry-pick $commit
then
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

echo "Cherry-pick failed. You are now in a subshell. Either resolve with git cherry-pick --continue or git cherry-pick --abort, then exit the subshell"
head_before=$(git rev-parse HEAD)
bash
head_after=$(git rev-parse HEAD)
if [[ "$head_before" = "$head_after" ]]; then
exit 1
fi
fi
git commit --amend -m "${message}${closes}"
else
git merge --no-ff --log=1000 FETCH_HEAD -m "Merge '$PR_TITLE' from $USER_NAME" -m "${PR_DESCR}${closes}"
fi
git commit --amend # for a manual double-check