scripts: add pull_github_pr.sh #2484

avikivity · 2024-10-09T17:25:10Z

Add a script that, given a pull request number, will merge it into the current branch, and add the pull request cover letter as a merge commit. This avoids githubs non-informative merge commits.

If the series contains a single patch, the patch is cherry-picked and the cover letter is discarded.

Adapted from scylladb.

Add a script that, given a pull request number, will merge it into the current branch, and add the pull request cover letter as a merge commit. This avoids githubs non-informative merge commits. If the series contains a single patch, the patch is cherry-picked and the cover letter is discarded. Adapted from scylladb.

tchaikov · 2024-10-10T00:31:17Z

scripts/pull_github_pr.sh

+
+curl() {
+    local opts=()
+    if [[ -n "$GITHUB_LOGIN" && -n "$GITHUB_TOKEN" ]]; then


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.

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.

tchaikov · 2024-10-10T00:31:45Z

scripts/pull_github_pr.sh

+fi
+
+if [ -z "$1" ]; then
+	echo Please provide a github pull request number


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

tchaikov · 2024-10-10T00:32:09Z

scripts/pull_github_pr.sh

+
+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} \


extraneous space after name.

tchaikov · 2024-10-10T00:32:31Z

scripts/pull_github_pr.sh

+PROJECT=`sed 's/git@github.com://;s#https://github.com/##;s/\.git$//;' <<<"${REMOTE_URL}"`
+PR_PREFIX=https://api.github.com/repos/$PROJECT/pulls
+
+echo "Fetching info on PR #$PR_NUM... "


extraneous space after ....

Or maybe he wanted "echo -n"?

tchaikov · 2024-10-10T00:33:22Z

scripts/pull_github_pr.sh

+echo "Fetching info on PR #$PR_NUM... "
+PR_DATA=$(curl -s $PR_PREFIX/$PR_NUM)
+MESSAGE=$(jq -r .message <<< $PR_DATA)
+if [ "$MESSAGE" != null ]


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

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

tchaikov · 2024-10-10T00:34:20Z

scripts/pull_github_pr.sh

+	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


tchaikov · 2024-10-10T00:35:23Z

scripts/pull_github_pr.sh

+set -e
+
+gh_hosts=~/.config/gh/hosts.yml
+jenkins_url="https://jenkins.scylladb.com"


this variable is not used.

tchaikov · 2024-10-10T00:35:36Z

scripts/pull_github_pr.sh

+
+gh_hosts=~/.config/gh/hosts.yml
+jenkins_url="https://jenkins.scylladb.com"
+FORCE=$2


this variable is not used.

tchaikov · 2024-10-10T00:35:56Z

scripts/pull_github_pr.sh

+gh_hosts=~/.config/gh/hosts.yml
+jenkins_url="https://jenkins.scylladb.com"
+FORCE=$2
+ORANGE='\033[0;33m'


this variable is not used.

tchaikov · 2024-10-10T00:36:08Z

scripts/pull_github_pr.sh

+jenkins_url="https://jenkins.scylladb.com"
+FORCE=$2
+ORANGE='\033[0;33m'
+NC='\033[0m'


this variable is not used.

nyh · 2024-10-10T05:50:16Z

Add a script that, given a pull request number, will merge it into the current branch, and add the pull request cover letter as a merge commit. This avoids githubs non-informative merge commits.

If the series contains a single patch, the patch is cherry-picked and the cover letter is discarded.

Adapted from scylladb.

Good idea. I actually copied the version of the script from Scylla before we started to add strange Scylla-CI-specific code to it, and have been using it in Seastar (and other projects) ever since, and it works very nicely :-)

nyh

Thanks. Looks good. Joining @tchaikov I only had a bunch of nitpicks.

nyh · 2024-10-10T05:53:47Z

scripts/pull_github_pr.sh

+
+set -e
+
+gh_hosts=~/.config/gh/hosts.yml


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

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

nyh · 2024-10-10T05:57:22Z

scripts/pull_github_pr.sh

+    exit 1
+fi
+
+if [ -z "$1" ]; then


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.

nyh · 2024-10-10T06:02:36Z

scripts/pull_github_pr.sh

+
+curl() {
+    local opts=()
+    if [[ -n "$GITHUB_LOGIN" && -n "$GITHUB_TOKEN" ]]; then


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.

nyh · 2024-10-10T06:07:10Z

scripts/pull_github_pr.sh

+	fi
+done
+
+curl() {


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.

nyh · 2024-10-10T06:09:44Z

scripts/pull_github_pr.sh

+echo "Fetching info on PR #$PR_NUM... "
+PR_DATA=$(curl -s $PR_PREFIX/$PR_NUM)
+MESSAGE=$(jq -r .message <<< $PR_DATA)
+if [ "$MESSAGE" != null ]


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

nyh · 2024-10-10T06:10:12Z

scripts/pull_github_pr.sh

+PROJECT=`sed 's/git@github.com://;s#https://github.com/##;s/\.git$//;' <<<"${REMOTE_URL}"`
+PR_PREFIX=https://api.github.com/repos/$PROJECT/pulls
+
+echo "Fetching info on PR #$PR_NUM... "


Or maybe he wanted "echo -n"?

tchaikov reviewed Oct 10, 2024

View reviewed changes

nyh requested changes Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: add pull_github_pr.sh #2484

scripts: add pull_github_pr.sh #2484

avikivity commented Oct 9, 2024

tchaikov Oct 10, 2024

nyh Oct 10, 2024

tchaikov Oct 10, 2024

tchaikov Oct 10, 2024

tchaikov Oct 10, 2024

nyh Oct 10, 2024

tchaikov Oct 10, 2024

nyh Oct 10, 2024

tchaikov Oct 10, 2024

tchaikov Oct 10, 2024

tchaikov Oct 10, 2024

tchaikov Oct 10, 2024

tchaikov Oct 10, 2024

nyh commented Oct 10, 2024

nyh left a comment

nyh Oct 10, 2024

avikivity Oct 10, 2024

nyh Oct 10, 2024

nyh Oct 10, 2024

nyh Oct 10, 2024

nyh Oct 10, 2024

nyh Oct 10, 2024


		set -e

		gh_hosts=~/.config/gh/hosts.yml

scripts: add pull_github_pr.sh #2484

Are you sure you want to change the base?

scripts: add pull_github_pr.sh #2484

Conversation

avikivity commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyh commented Oct 10, 2024

nyh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment