Add workflow for automated PR linting by vilmibm · Pull Request #3193 · cli/cli

vilmibm · 2021-03-10T20:10:15Z

This PR adds a single workflow to be run on pull request open, reopen, and ready for review.

If the PR is authored by a CLI core team member, it's automatically added to the Needs Review column.
If it's not, this workflow checks:
- if the PR has a substantial body, closing if not
- if the PR is not targeting trunk, closing if not
- if the PR does not mention an issue, warning if not.

If the community PR passes each check, it's added to Needs Review.

All of this runs under a new GH account, cliAutomation. You can see it in action (closing its own PR lol) in my testing repo

NB: I also wanted to include a check for whether or not a PR had any "real" commits from the PR author since a common spam PR approach is opening PRs full of the commits of others. This ended up being very annoying to determine since linking from a GitHub username (all I know about a PR author) to a commit (which just has name and email) is not easy. I noticed all of our recent spam would have been caught by either of the other two checks, so I punted for now.

Open questions

Is merely warning about lack of issue number sufficient?
Is adding directly to Needs Review if a community PR passes this linting correct, or do we merely want to leave it open for a first responder to notice?
Should this ignore draft PRs?

vilmibm · 2021-03-11T16:53:52Z

Apologies for the reopening spam -- I ran into a permission problem on cli/cli's project board I didn't encounter in my testing repo and had to do a bunch of trial and error x_x this is ready for review.

mislav

This is great! Sorry about being nitpicky about bash in review comments 🙈

Do we have any sort of guard against repeated comments? E.g. let's say that I open a PR without a body. It gets closed. Then I reopen it before I addressed all the requirements; e.g. I put some text in the body, but I didn't reference any issue. Now I will get another comment from the bot. I guess the 2nd one is fine, but I wonder should there be a guard against some sort of comment loop that a bot could get stuck in.

mislav · 2021-03-12T13:00:49Z

+    steps:
+      - name: lint pr
+        env:
+          REPO: "cli/cli"


If this is called GH_REPO instead, then we don't have to explicitly add -R$REPO to any command.

mislav · 2021-03-12T13:06:46Z

+
+          addToBoard () {
+            # this exec is an attempt to fix some quote highlighting issues i was having in my editor.
+            bash -c 'gh api graphql -f query="mutation { addProjectCard(input: { projectColumnId: \"$NEEDSREVIEWCOL\", contentId: \"$PRID\" }) { clientMutationId } }"'


Passing graphql variables as separate arguments avoids quoting issues:

Suggested change

bash -c 'gh api graphql -f query="mutation { addProjectCard(input: { projectColumnId: \"$NEEDSREVIEWCOL\", contentId: \"$PRID\" }) { clientMutationId } }"'

gh api graphql -f query='mutation($col:ID!, $pr:ID!) { addProjectCard(input: { projectColumnId: $col, contentId: $pr }) { clientMutationId } }' -f col="$NEEDSREVIEWCOL" -f pr="$PRID"

mislav · 2021-03-12T13:08:41Z

+            commentPR "Thanks for the PR! We're a small team and it's helpful to have context around community submissions in order to review them appropriately. Our automation has closed this PR since the body appears to be lacking much content; please add a more descriptive PR body as well as any issues it closes and reopen. Thanks again!"
+            closePR


Feels like we should resurrect #1038 🤔

mislav · 2021-03-12T13:09:43Z

+          NEEDSREVIEWCOL: "MDEzOlByb2plY3RDb2x1bW43MTEwMTI4"
+          PRID: ${{ github.event.pull_request.node_id }}
+          PRBODY: ${{ github.event.pull_request.body }}
+          STAFF: "vilmibm,mislav,samcoe,billygriffin,ampinsk"


Could we grab this dynamically?

gh api orgs/cli/members -q '.[].login'

mislav · 2021-03-12T13:19:29Z

+            bash -c 'gh api graphql -f query="mutation { addProjectCard(input: { projectColumnId: \"$NEEDSREVIEWCOL\", contentId: \"$PRID\" }) { clientMutationId } }"'
+          }
+
+          if echo $STAFF | grep $PRAUTHOR


If the only reason we allow-list STAFF is so that we can check whether the author is among them, maybe we can have a more direct check?

This command will succeed if vilmibm is a member of our org:

gh api orgs/cli/public_members/vilmibm --silent 2>/dev/null

mislav · 2021-03-12T13:29:33Z

+
+          if echo "${PRBASE}" | grep -Ev '^trunk$'
+          then
+            commentPR "This pull request should probably be targeting trunk; since it isn't, our automation has closed it. Please reopen with an appropriate base branch if this was in error."


Hmm I'm not sure about requiring that every contributor PR must have trunk as base. I do not see why someone should be blocked to submit a PR that is based on someone else's branch (e.g. a chained PR).

Furthermore, are the instructions how to fix this even feasible? Can one change the base branch of a closed PR before reopening it?

Should we block PRs that have trunk as the head branch? Those are definitely spammy.

mislav · 2021-03-12T13:36:42Z

+
+          if echo "${PRBODY}" | grep -Ev '#[0-9]+'
+          then
+            commentPR "Hi! Thanks for the PR. Please ensure that this PR is related to an issue by mentioning its issue number in the PR body. If this PR would close the issue, please put the word 'Fixes' before the issue number somewhere in the PR body. If this is a tiny change like fixing a typo, feel free to ignore this message."


This is a great warning message ✨

Should we link to our docs about the "fixes" keyword?

Since we avoid the acronym "PR" in our docs, should we do it here as well? A newcomer to GitHub might not immediately connect the docs about what a "PR" is.

mislav · 2021-03-12T13:38:08Z

+          STAFF: "vilmibm,mislav,samcoe,billygriffin,ampinsk"
+          PRNUM: ${{ github.event.pull_request.number }}
+          PRBASE: ${{ github.event.pull_request.base.ref }}
+          GH_TOKEN: ${{ secrets.AUTOMATION_TOKEN }}


A may have missed this, but what was the rationale for creating a dedicated user for automation instead of using GITHUB_TOKEN here and have the commenter be "github-actions"?

mislav · 2021-03-12T13:45:21Z

+            exit 0
+          fi
+
+          if echo "${PRBODY}" | grep -Ev '#[0-9]+'


By default, grep will print lines matched—or, in this case, lines that don't have a match. The --quiet flag suppresses it.

Furthermore, even though the -v flag inverts the match, it does not invert the logic of the exit status of grep. Grep will continue to exit with zero status if any lines were printed. This condition will basically be true for any multi-line PR body, and the only way for it to be false is if the body is empty or if every line contains a #123 expression.

Here's a condition that I think will work:

Suggested change

if echo "${PRBODY}" | grep -Ev '#[0-9]+'

if ! grep -Eq '#[0-9]+' <<<"$PRBODY"

It also reads better: "if there is no match, do this".

mislav · 2021-03-12T13:48:00Z

+            # TODO should this be close-worthy? I feel like sometimes it's ok
+            # to not have an issue (like a tiny typo fix) so we should have
+            # this be a warning.


Nah, let's just leave it was a warning for now.

- Do not add draft PRs to the review board - Do not enforce that the base branch must be "trunk" - Refuse PRs made with our "trunk" as the head - Improve staff check to avoid hardcoding - Improve pattern matching when suggesting to link to an issue - Use the stock GITHUB_TOKEN

mislav

Pushed some updates to this one 👌

add workflow

75ac259

vilmibm requested review from billygriffin, mislav and samcoe March 10, 2021 20:10