Don't trigger PR limit reminder on PRs that are ignored

[AetherUnbound]

Description

This PR attempts to fix an issue wherein our PR limit reminder bot will ping you even if your most recent PR is one of the PRs that can be safely ignored for the check (e.g. documentation or critical). While the count only includes the non-ignored PRs, the ping is still issued if the user submits another, ignorable PR. I found this issue because I submitted 3 documentation PRs over the limit, but each bot reminder said "you have 4 PRs open".

Testing Instructions

I have no idea if this works or is even valid! Opening this to try and find out :) (CC @dhruvkb as you might have the most familiarity with GHA)

Checklist

• My pull request has a descriptive title (not a vague title likeUpdate index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.
• I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[AetherUnbound]

I just got pinged again for this PR 😆 which means that drafts also make this happen, so I'll need to rework this a bit to make it more resilient to the other checks we make.

[sarayourfriend]

I think this is a correct change conceptually. I am requesting changes just to shortcut making any requests to save us on the GitHub rate limits and (I think) also really significantly simplify this by-pass edge case. Even in reviewing this PR, the way this fixed the issue was inscruitble to me for almost 10 minutes while I read and re-read the code. Reading it after the fact, I don't think I'd ever understand that this edge case was handled in the way that it is. Adding an early return and avoiding the entire operation would be clearer as well as avoiding the rate limit hit.

[sarayourfriend]

The event that triggers this script already has the PR in context (https://docs.github.com/en/webhooks/webhook-events-and-payloads#pull_request) from which we can retrieve the latest PR. @dhruvkb told me GitHub's rate limits for GraphQL are a bit different (https://docs.github.com/en/graphql/overview/rate-limits-and-node-limits-for-the-graphql-api#primary-rate-limit) so anything we can avoid requesting would be good.

Instead of getting the most recent PR this way, we can pass the pull_request context object to the script and check if it's a relevant PR at the top, and avoid making any requests at all if it isn't. Something like:

def (..., pullRequest):
  if not isRelevantPR(pullRequest):
    return { pr_count: 0 }

  # the rest of the function

[AetherUnbound]

It looks like we have a few helper methods for getting the pull request object in other scripts:

openverse/automations/js/src/label_pr.mjs

Lines 57 to 72 in 8b0ec49

	const { eventName, eventAction, prNodeId } = JSON.parse(
	readFileSync('/tmp/event.json', 'utf-8')
	)
	if (
	eventName !== 'pull_request' ||
	!['opened', 'edited'].includes(eventAction)
	) {
	core.info(
	`Event "${eventName}"/"${eventAction}" is not an event where a PR should be labelled.`
	)
	return
	}
	const pr = new PullRequest(octokit, core, prNodeId)
	await pr.init()

openverse/automations/js/src/project_automation/prs.mjs

Lines 67 to 75 in 8cd1dbc

	const { eventName, eventAction, prNodeId } = JSON.parse(
	readFileSync('/tmp/event.json', 'utf-8')
	)
	core.debug(`Event name: ${eventName}`)
	core.debug(`Event action: ${eventAction}`)
	core.debug(`PR node ID: ${prNodeId}`)
	const pr = new PullRequest(octokit, core, prNodeId)
	await pr.init()

These use GraphQL under the hood themselves, so I'm not sure we'd change cut down much on usage by using this method instead. Is it really as simple as putting pull_request in the exports here?

openverse/automations/js/src/count_user_reviewable_prs.js

Line 11 in b8230be

module.exports = async ({ github, context, core }) => {

Do you know where I might be able to find the shape of the object that's available when I do? I'm struggling with the documentation for Octokit 😅

[sarayourfriend]

No, you'd need to get it in from the GitHub action context. You might be able to get it from context.payload.pull_request, and that might have the same shape as the github.event.pull_request present in the YAML contexts, which I found a dump of via this SO answer:

https://gist.github.com/colbyfayock/1710edb9f47ceda0569844f791403e7e#file-github-context-json-L19

So something like context.payload.pull_request.created_at could get you the creation date.

Those other scripts you found are pulling the PR information out of a temporary file built from the event context, and is only necessary because of what Dhruv explains in this comment:

openverse/.github/workflows/pr_automations_init.yml

Lines 1 to 18 in 889c3b5

	# Initialises all automations related to PR events.
	#
	# See `issue_automations.yml` for the corresponding implementation for issues.
	#
	# The automations for PR events are a little more complex than those for issues
	# because PRs are a less secure environment. To avoid leaking secrets, we need
	# to run automations with code as it appears on `main`.
	#
	# `pull_request_target` serves this purpose but there is no corresponding
	# `_target` version for `pull_request_review`. So we take this roundabout
	# approach:
	#
	# 1. This workflow runs for the events and their subtypes we are interested in.
	# 2. It saves the event name, action and PR node ID to a JSON file.
	# 3. It uploads the JSON file as an artifact.
	# 4. Its completion triggers the `pr_automations.yml` workflow.
	#
	# continued in `pr_automations.yml`...

It isn't relevant for this particular script because this script only runs on pull_request_target and no other event type, and so will always have the pull_request in the event context (unlike pull_request_review).

If for some reason context.payload.pull_request is not a full pull_request object, then you can add it as a property on the module.exports = async ({ github, context, core }) => { you shared as:

 module.exports = async ({ github, context, core, triggeringPullRequest }) => { 

and then pass that in the workflow file like so:

openverse/.github/workflows/pr_limit_reminders.yml

Line 24 in 889c3b5

await script({github,context,core})

to

await script({ github, context, core, triggeringPullRequest: ${{ toJSON(github.event.pull_request) })

I'm hoping you don't need to do that though and that we can get it from context.payload.pull_request. I'm sharing this alternative just in case the simpler approach doesn't work. Unfortunately GitHub Action contexts are not super well documented (as you can see from that SO question I linked).

[AetherUnbound]

Thanks for all that info! And amazing find with that gist. I think I've got a workable approach in dfa9759 - I had to add a separate function for checking relevancy since the shape of the context's PR is different from the one returned by the GraphQL API. Let me know what you think!

[sarayourfriend]

LGTM!

[sarayourfriend]

Nice! This is much easier to understand, at least to me 🙂

[dhruvkb]

LGTM! This is a welcome improvement to these notifications! I have bikeshedding nits over the variable names so feel free to address or dismiss them, nothing worth blocking over.

While I appreciate the handling of pagination in the GraphQL call, my YAGNI-sense is tingling because we never reach that many open PRs and there are many other pieces of code where we do not do this. Losing it can significantly reduce the complexity.

[dhruvkb]

Nit:

Suggested change

	const isRelevantPRFromGraphQL = (pr) =>
	const isRelevantPrFromGraphql = (pr) =>

I don't want to block a PR for a variable name, but I also want to avoid creating something similar to XMLHttpRequest.

As for GraphQL, the most common identifier I've seen for it is 'graphql' (all lowercase), so just uppercasing the 'G' for the camelCase version makes sense to me.

[AetherUnbound]

Updated!

[dhruvkb]

This should be fine based on the shape of Context: https://github.com/actions/toolkit/blob/main/packages/github/src/context.ts