Add `delete_pr_comment`

[crazytonyli]

A new script to delete PR comments that contain a given content.

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 delete comments (or not) based on the --containing argument.

---

• [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]

Comment test 1

[mokagio]

Comment test 3

[crazytonyli]

@mokagio Regarding your concern on deleting other comment by accident. I definitely agree with you. That's on us to make sure the query criteria is unique enough, i.e. using an invisible code block like you mentioned. I extracted this script from an automation tool in one of our private repo. That tool posts and deletes comments that has certain invisible block content.

But I have added a restriction in https://github.com/Automattic/a8c-ci-toolkit-buildkite-plugin/pull/52/commits/0cdfa64dc528ea9c486b6f1f1a02e1338b9af15b, to only allow query <!-- xxx -->, which should reduce the chances of deleting wrong comments.

[AliSoftware]

Late to the party, but instead of adding this delete_pr_comment command, with the dangerous side effect that Gio highlighted, I'd argue to instead make the comment_on_pr command accept a commentID param, and then allow that comment_on_pr command to delete a comment if the body we pass to it is empty.

That way, not only will we guarantee that we can only delete comments by commentID (and not arbitrary containing: test), but also we'd only have one source of truth for how that magic HTML comment is formatted and how to get a GitHub comment handle from the comment id.

---

@crazytonyli Given all that, I'd advocate to revert that PR / remove that new delete_pr_comment command ASAP before we ship a new a8c-ci-toolkit version (otherwise if we release a new version before deleting the command, and delete that command only later, we'll need to do a major version bump…), and instead move the logic to delete an existing comment into comment_for_pr, as part of the work being done in https://github.com/Automattic/a8c-ci-toolkit-buildkite-plugin/pull/51#issuecomment-1534855103.

Doing that, the cal site would then look like this:

export GITHUB_TOKEN=…
comment_on_pr --id "terraform-plan-aws" "Go to CI and click on the button to apply the plan if it looks ok" # add a new comment
comment_on_pr --id "terraform-plan-aws" "The plan has now been applied into production" # update existing comment
comment_on_pr --id "terraform-plan-aws" "" # Delete an existing comment completely

[crazytonyli]

@AliSoftware One side effect with this approach is, the previous comment, which potentially contains outdated and inaccurate information, would only be deleted at the time a new comment is updated or posted. Continuing with your example, let's say the terraform plan step failed, what you'd see on PR is a failed "terraform-plan" CI check and a comment saying "here is the terraform plan" with an outdated plan.

In the PR where this delete_pr_comment was originated from, I initially took the "updating the existing comment" approach you mentioned, but ended up going with deleting the comment first before creating a new one, because it can produce the most accurate information.

But I'm happy to remove this delete_pr_comment from this plugin though, deleting a comment probably isn't a common thing we do. It should be fine just keep the script in the repo that needs it.

[AliSoftware]

That's a valid point.

And to be fair that's a limitation that we already have with the comments we do on PRs of app repos to announce where to download Prototype Build for the PR: if the first CI build succeeded in building the Prototype Build, but q subsequent commit and need CI build failed to build the Prototype Build on the new commit, the old comment pointing to the old Prototype Build would not be dismissed while in practice it would not represent the reality of "the Prototype Build containing every changes that appears on this PR" (but instead, every change on this Pr except last commit that failed to build)

That being said, nothing prevents us to implement the capability for comment_on_pr --id 'terraform-plan-aws' "" to delete a comment when the provided comment body is empty (or when we pass --delete flag or whatever), and then call that as an in-place replacement of delete_comment_pr (ie before running terraform plan to ensure old comment got deleted even if terraform plan failed.

It'd just be an implementation change detail (of having this feature from this delete_pr_comment being now implemented as comment_on_pr --delete instead). But the big advantage of doing such a change being that we'd now guarantee that the logic to add the <!-- comment-id:${COMMENT_ID} --> hidden HTML comment to identify a GH comment would be handled directly by comment_on_pr instead of having the caller to do that dance, and then have the guarantee that the logic to find which comment to delete on comment_on_pr --id xyz --delete would rely on then same format for the <!-- comment-id:${COMMENT_ID} --> bit in both the use case of adding and deleting a comment (instead of having to replicate the HTML comment format assumption in the implementation of both comment_on_pr --id someid and delete_pr_comment --id someid if we were to keep those feature in separate commands/implementation like here)

Wdyt?

[crazytonyli]

That sounds good to me! I'll make the change then. It didn't occur to me that we no longer need the delete_pr_comment, because comment_on_pr with an empty comment does that too.

[AliSoftware]

Closing the loop: deletion of this new delete_pr_comment applied in #53, while the ability to delete a PR comment has been moved into the implementation of #51.