Re-implement `comment_on_pr` to use `curl` instead of `gh`

[crazytonyli]

The existing script depends on gh and macOS (installing gh when it's missing). I've re-implemented to make it compatible to Linux too, as long as curl and jq are installed.

I recorded this as an internal change, since the interface is backwards compatible.

Test instructions

Go to this PR's buildkite job page, select a step, and go to the "Environment" tab. Click the "Show export prefix" button and copy the environment variables. Open a terminal on your computer and paste the copied env vars. Invoking this script from your shell session should post a comment to this PR, or fail with an non-zero exist code.

---

• [x] I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

[mokagio]

test comment from Gio

[mokagio]

My comment above was posted with a modified version of this script hacked to test the suggestions I made

diff --git a/bin/comment_on_pr b/bin/comment_on_pr
index 6b01903..a957f74 100755
--- a/bin/comment_on_pr
+++ b/bin/comment_on_pr
@@ -1,4 +1,4 @@
-#!/bin/bash -eu
+#!/bin/bash -eux

 # This script is used to comment on a pull request, and must be run in a buldkite PR step.
 #
@@ -18,16 +18,24 @@ fi

 GITHUB_TOKEN=$2

-GITHUB_URL="${BUILDKITE_PULL_REQUEST_REPO%.git}"
+URL_WITHOUT_SUFFIX="${BUILDKITE_REPO%.git}"
+GITHUB_URL="${URL_WITHOUT_SUFFIX#git@github.com:}"
 GITHUB_USER=$(basename "$(dirname "$GITHUB_URL")")
 GITHUB_REPO=$(basename "$GITHUB_URL")

+# TODO: What to do if there is more than one PR?
+# TODO: Maybe this could be a fallback if BUILDKITE_PULL_REQUEST is false, not the default approach?
+PR=$(curl -s -X GET --fail-with-body -H "Authorization: token ${GITHUB_TOKEN}" -H "Content-Type: application/json" -H "Accept: application/vnd.github+json" "https://api.github.com/repos/${GITHUB_USER}/${GITHUB_REPO}/commits/${BUILDKITE_COMMIT}/pulls" | jq '.[0].number')
+
+# FIXME: Fail if there is no PR
+
 JSON_PAYLOAD=$(jq --null-input --arg body "$PR_COMMENT" '{body: $body}')

 curl -s -X POST \
-    --fail-with-body \
-    -H "Authorization: token ${GITHUB_TOKEN}" \
-    -H "Content-Type: application/json" \
-    -H "Accept: application/vnd.github+json" \
-    -d "${JSON_PAYLOAD}" \
-    "https://api.github.com/repos/${GITHUB_USER}/${GITHUB_REPO}/issues/${BUILDKITE_PULL_REQUEST}/comments"
+  --location \
+  --fail-with-body \
+  -H "Authorization: token ${GITHUB_TOKEN}" \
+  -H "Content-Type: application/json" \
+  -H "Accept: application/vnd.github+json" \
+  -d "${JSON_PAYLOAD}" \
+  "https://api.github.com/repos/${GITHUB_USER}/${GITHUB_REPO}/issues/${PR}/comments"

The --location was a leftover from a previous attempt to follow a redirect. When using BUILDKITE_REPO, the value was the legacy bash-cache... URL. Unfortunately, the command still failed, but I now realize it was because the commit I was using had no PR. The solution I found for that was to update the URL for this pipeline in the settings so that the GitHub API would not redirect. We might want to support redirect in the future, though.

[crazytonyli]

Given this script is designed to be called only in PR jobs, I think we should reply on those env vars only present in PR jobs? Because technically there are difference between a job that builds a branch and a job that builds a PR.

E.g. notice how only the build at the top of the list here has a PR associated with it:

I think that might be related to this project's buildkite pipeline settings. Buildkite kicks off a branch job when new branch is created or new commits is pushed. If a PR is created after the branch job has completed, buildkite doesn't start an new PR build. That's probably because the "Skip pull request builds for existing commit" is turned on.

If you search for "skip_pull_request_builds_for_existing_commits" in one of our private repos (that shall not be named 😄 ), you'll see some projects (like wordpress-ios and woocommerce-ios) have this option turned off.

$ fgrep skip_pull_request_builds_for_existing_commits -r buildkite/pipelines --no-filename | sort | uniq -c
  30     skip_pull_request_builds_for_existing_commits = false
  84     skip_pull_request_builds_for_existing_commits = true

I think it would be good to review this at some point and see if it's necessary to have different settings on our projects.

[AliSoftware]

Given this script is designed to be called only in PR jobs, I think we should reply on those env vars only present in PR jobs? Because technically there are difference between a job that builds a branch and a job that builds a PR.

I agree with this.

I think it would be good to review this at some point and see if it's necessary to have different settings on our projects.

Totally, and this is already in our backlog to review it all at some point anyway (with your project to manage Buildkite settings via terraform and all). Personally I suggest we don't create builds for commits not associated with Pull Requests, except for the long-running branches (trunk/develop/…). If one want to have CI test their work in progress even if they're not ready to have the work reviewed yet they can always create a Draft PR anyway. On the other end, letting CI create builds for commits without a PR all while having the "cancel previous builds" enabled (which we want) will often make the CI check first turn red on the PR (because the build for the commit push would have been created first, then immediately cancelled and marking the PR red) only to later finish the 2nd build (the one associated with PR creation) and running the CI steps again and hopefully finalluy turning them back green.

See also private discussions in Slack: p1678808720955539/1678807631.262069-slack-CR5E8MXLZ and p1677163098116359/1677160312.855239-slack-CR5E8MXLZ

[mokagio]

Given this script is designed to be called only in PR jobs, I think we should reply on those env vars only present in PR jobs? Because technically there are difference between a job that builds a branch and a job that builds a PR.

Sounds good to me. I'd still be keen to add an explicit check for BUILDKITE_PULL_REQUEST and BUILDKITE_PULL_REQUEST_REPO in the script, though, so that if they are missing we can let the "user" know about it. What do you think?

[crazytonyli]

I'd still be keen to add an explicit check

For sure. Added in https://github.com/Automattic/a8c-ci-toolkit-buildkite-plugin/pull/51/commits/1bf308501b9987300def5233379ad262579c3588

[AliSoftware]

I'd love for this action to evolve so that it can include a comment id, allow for updating an existing comment with that comment id (instead of adding a new comment) if one exists already, and allow to delete an existing comment by passing an empty comment body.

Kinda similar to how our equivalent comment_on_pr fastlane action is doing it in release-toolkit

_imho it'd make more sense to have a single comment_on_pr command like that here that handles everything rather than having multiple commands (comment_on_pr and delete_pr_comment etc)—especially if we want comment_on_pr to already be able to delete/update an existing comment if one with passed commentID was found, then no need to implement the deletion logic twice._

See https://github.com/Automattic/buildkite-ci/pull/170#discussion_r1185077675 for a typical place where this could be useful and allow some nice DRYing, as well as this comment on recently-merged delete_pr_comment PR.

[crazytonyli]

@AliSoftware I'm happy to add this additional feature. However, as I mentioned in https://github.com/Automattic/a8c-ci-toolkit-buildkite-plugin/pull/52#issuecomment-1535472163, adding this feature won't be able to replace how some use cases use delete_pr_comment before comment_on_pr.

[AliSoftware]

@AliSoftware I'm happy to add this additional feature. However, as I mentioned in https://github.com/Automattic/a8c-ci-toolkit-buildkite-plugin/pull/52#issuecomment-1535472163, adding this feature won't be able to replace how some use cases use delete_pr_comment before comment_on_pr.

I think it still would, given that my suggestion above also included the mention:

[…] and allow to delete an existing comment by passing an empty comment body.

So all you'd have to do is call comment_on_pr --id terraform-plan-aws "" (*) as an in-place replacement for the places where you currently wanted to call delete_pr_comment.

_(*) This was just an API suggestion amongst others. Now that I think more about it, maybe making the command API looking more like comment_on_pr --id xyz --delete (rather than relying on empty body param) would be better for this use case, up to you_

[crazytonyli]

@AliSoftware Thanks for the heads up. I thought about that and added a --if-exist update|delete option.

[crazytonyli]

@mokagio @AliSoftware I've requested a review since there are quite lots a new changes. Please see the "Usage" comment in the script for some use cases. I have verified all use cases work. But please feel free to give it a go and let me know if you find any issues. Thanks!

[AliSoftware]

@AliSoftware says: "testing, 1,2,1,2"... I'll add some $special characters too