ci: improve and automate issue/PR labels

[wexxlee]

(edited on 03-01-2024)

With cactbot's transition to a community-maintained project, I'd like to propose some changes to the issue/PR labels to help with organization and automation. This PR includes a handful of workflow scripts an update to the existing PR labeling workflow script to automate some of the label management discussed below.

I'm admittedly trying to strike a balance between adding things that will be useful and will keep the issue/PR lists scrubbed (especially if/when repo activity ramps up leading in to 7.0), but not overengineering. I'm not sure how well this proposal strikes that balance, so this is definitely a wide call for comments, suggestions, and pushback.

NB: If we decide to go forward with this (or some version of this) proposal, it probably makes sense to throw this in a 'maintainers' guide doc or something similar. But I'd be inclined to tackle that with a follow-up PR.

Issue Labels:

Existing issue type labels (bug, question, enhancement) are automatically applied when a new issue is opened. needs-review is also automatically applied to all new issues (and can be manually removed once an issue is reviewed).

I propose to keep that behavior, and add the following label options:

Label	Applied	Intent
pending info	Manual	Used when we are waiting on more info from the issue opener.
cant repro	Manual	Used when a bug cannot be reproduced and appears unique to the issue opener.
fix-me	Manual & Automatic	Used to indicate a valid bug/feature request that should be addressed. (Auto-applied when issue linked to PR.)
not planned	Manual	Used to indicate that a bug/requested feature is intended behavior, a system limitation, etc., and won't be addressed.
stale	Automatic	Applied automatically to inactive issues (see discussion below) after a period of inactivity. If there is no further activity, they will be automatically closed.

PR Labels:

Similarly to issues, I propose to automatically label PRs with needs-review when opened and whenever a new commit is pushed to the PR (until an approving review is submitted). That label can be manually removed by the reviewer once the PR has been initially reviewed will be automatically removed whenever a maintainer submits a review (approving or otherwise).

I also propose to add the following label options:

Label	Applied	Intent
auto-merge	Automatic	Toggled whenever a PR's auto-merge setting is changed (added visibility for reviewers before approving).
changes-requested	Manual	Used when a reviewer has requested changes before merging. Can be manually removed once changes are committed & reviewed.
review-complete	Automatic	Automatically applied when an approving review is submitted.
stale	Automatic	Same as with issues (see discussion below).

Supporting Workflow Scripts:

This PR modifies the existing (now renamed) labels.yaml -> label_pr.yml script to automate this functionality, specifically:

• If a PR is opened or a new commit is pushed:
  • auto-label.cjs will (continue to) add scope-related labels
  • auto_label.cjs will add needs-review unless the PR already has an approving review from a maintainer
  • Any linked issues will have the fix-me label applied to them.
• If a review (approving or otherwise) is added by a maintainer, the needs-review label is removed.
• If auto merge is enabled or disabled on the PR, the auto-merge label is added/removed.

This PR includes some workflow scripts to help automate the label management discussed above.

• label-linked-issues - adds fix-me to an issue when it is linked to a PR.
• label-pr-automerge - toggles auto-merge on a PR whenever the auto-merge setting is changed.
• label-pr-review -
  • Adds review-complete (and removes needs-review) when an approving review is submitted.
  • Adds needs-review to a PR when opened or when a new commit is pushed (unless review-complete is present).
• stale-daemon - runs actions/stale every 4 hours and:
  • Adds stale to any issue that has had no activity in 30 days and does not have needs-review or fix-me.
  • Adds stale to any PR that has had no activity in 60 days and does not have needs-review.
  • Removes stale from any issue/PR if there is subsequent activity.
  • Closes any issue/PR that has been labeled stale for 7 days with no subsequent activity.
  • Adds an explanatory comment to issues/PRs when marking as stale or closing.

[valarnin]

Some initial impressions:

• stale: I don't think this makes sense in the current context and scope of the project. If we start accumulating multiple pages of issues or PRs, then maybe we revisit this? I know that the quisquous/cactbot repository ended up with a lot of old issues but I feel like that was more a lack of cleanup in general and that we should be more proactive about following up on issues and PRs?
• changes-requested: This is sort of automatically handled by github already. If a reviewer selects the appropriate option when reviewing, it'll flag the review as requiring changes and notify via email (I believe).
• review-completed: Same as the changes-requested one, this seems redundant since the review status is easily visible from the PRs page.

I'd like to give this some more time to percolate in my mind before confirming above thoughts or giving further opinions.

[JLGarber]

I'm in agreement with @valarnin regarding the use of stale labeling. While there were definitely a lot of old issues in quisquous's repository, it was a More Than Once occurrence where we would fix something a year later. As a baseline for reconsideration, let's say maybe if we start a second page of PRs with the oldest being more than 2-4 weeks old?

(As a personal take on the use of the label, I feel like it tends to end up kind of user-hostile, and its implementations usually haven't been helpful in the situations I've observed. I would prefer to avoid that here unless it's really necessary.)

Apart from that, I'm fine with the rest of the changes and will defer to you all on the specifics of any implementations.

[xiashtra]

Similar thoughts to @valarnin on stale, changes-requested, and review-completed. The rest look fine to me.

[wexxlee]

Appreciate all the feedback so far. I'm planning to leave this open at least until next weekend in case others want to chime in or if there are additional thoughts.

• stale - All good points. I'm planning to take it out, and we can revisit at some point if it ever seems like there's more of a need.

• changes-requested - I think valarnin's point is a good one. The only value this really adds is that you can easily see from the PR list whether a PR has been reviewed & changes have been requested without needing to drill into the PR. But that's probably also apparent if the PR is not longer tagged as needs-review, or if there are comments on it, so in retrospect, maybe this really doesn't add much if any value. Leaning toward dropping it.

• review-complete - this definitely is redundant since you can see from the PR list whether a PR has been approved. I intended this solely as a crutch, to prevent needs-review from being reapplied to an approved PR (with the intent of conforming to our sometimes-approach of approving a PR with suggestions that the submitter is free to take-or-leave before merging). Whether a PR is approved isn't tracked as state attribute in the GitHub REST API, hence the need for a label as a state crutch -- but digging a little more, I think I've found a way to track approvals from the pull event history, so if I can validate that, then I see no reason to keep this one. (Edit: Yeah, it works. This can be dropped too.)

[wexxlee]

Updated the PR based on the comments above, and edited my original description to reflect the changes. This also consolidates all of the PR labeling functionality into the existing (renamed for clarity) workflow, which should be easier to review/maintain.

[valarnin]

This looks good to me now.