Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GH action to enforce PR labels #52760

Merged
merged 4 commits into from Jul 26, 2023
Merged

Conversation

priethor
Copy link
Contributor

@priethor priethor commented Jul 19, 2023

What?

This PR introduces a new GitHub action that enforces that all PRs have exactly one label indicating the type of PR; other labels are not considered in this check.

When a PR has less or more than 1 type-related PR, the bot leaves a comment, and pre-merge checks fail. When the labels are updated, the bot updates/removes the comment and pre-merge checks accordingly.

Why?

Working on the Gutenberg changelog with mislabeled PRs is challenging. Some examples include:

  • PRs with no label indicating their nature.
  • PRs with conflicting labels; bugfixes and enhancements refer to the product and not the codebase, like, for example:
    • A PR fixing a documentation issue test should be a documentation PR, not a bugfix.
    • A PR improving an e2e test is an automated testing PR, not an enhancement.
  • PRs with unclear labels, like Task (at a high level, all PRs are tasks 😅) or that don't apply to PRs, only to issues (like Overview, Tracking issue)

How?

By adding an automation that checks labels and enforces using exactly one of a small set of them, as follows (to be updated with the final labels used):

  • [Type] Accessibility (a11y)
  • [Type] Automated Testing
  • [Type] Breaking Change
  • [Type] Bug
  • [Type] Build Tooling
  • [Type] Code Quality
  • [Type] Copy
  • [Type] Developer Documentation
  • [Type] Enhancement
  • [Type] Experimental
  • [Type] Feature
  • [Type] New API
  • [Type] Task
  • [Type] Performance
  • [Type] Project Management
  • [Type] Security
  • [Type] WP Core Ticket

Note that while there are more type-related labels, many of them don't apply to PRs (e.g., Overview, Discussion don't apply, or PRs that fix Flaky Test issues can be consolidated as [Type] Automated Testing PRs).

Testing Instructions

Check my test repo with the automation enabled here.

Screenshots or screencast

The screenshot below shows what the automation would look like, but please note the example labels are not the same ones as in the Gutenberg repository. See the How above to see which labels would be enforced in this repository.
image

@priethor priethor added the [Type] Project Management Meta-issues related to project management of Gutenberg label Jul 19, 2023
@priethor priethor requested review from ockham, jordesign, mrfoxtalbot and a team July 19, 2023 12:18
@priethor priethor self-assigned this Jul 19, 2023
@juanmaguitar juanmaguitar requested a review from ndiego July 19, 2023 12:25
@github-actions
Copy link

github-actions bot commented Jul 19, 2023

Flaky tests detected in 364cc27.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5656170550
📝 Reported issues:

@priethor priethor requested a review from apeatling July 19, 2023 16:08
@annezazu
Copy link
Contributor

I'm confused about [Type] Accessibility - would this be changing the overall Accessibility label to that? We're trying to consolidate right now so just wanted to check. In my mind, only having one of these wouldn't be enough for accessibility. For example, it's more likely that we'd see [Type] Accessibility & [Type] Bug. Perhaps I'm missing something though :D

@priethor
Copy link
Contributor Author

I'm confused about [Type] Accessibility - would this be changing the overall Accessibility label to that?

Good point; I should have added a note there: I think the Accessibility type should be a type by itself, as it is [Type] Performance. Similarly, in the Gutenberg changelog, both accessibility and performance items are grouped in a top-level section and not included under bugs or enhancements. A11y and performance are not features that are either working or not; they are never finished and in my opinion, should be a category on their own.

In any case, it's only my subjective opinion. I don't have strong opinions and would like to hear @alexstine and @afercia's opinions on this! I'm happy to go either way, and whether we make Accessibility a type of task or not, we will make sure the changelog automation puts a11y items in its own top-level section. 🙏

@afercia
Copy link
Contributor

afercia commented Jul 20, 2023

Thanks for all your work on this reorganization. I don't have a strong opinion on whether accessibility should be a type.

If that can help, I'd like to provide some context on why Accessibility in core is a Trac 'focus' and not a Trac component. It dates to a big reorganization of Trac by @nacin in 2014. Emphasis mine:

Continuing the Trac component re-organization with "focuses"

A new concept: Focuses

We’ve also added seven new “focuses” — ui, accessibility, javascript, docs, multisite, performance, and rtl. Focuses are about broad concepts and help break tickets down by specialties and skills, rather than functional areas of core. Multisite and RTL are widely general “modes” for WordPress, and each have contributors who focus strongly on those areas, but they are not well-contained “components” of core. Accessibility isn’t an area of core — it permates the entire user experience.

@ndiego
Copy link
Member

ndiego commented Jul 24, 2023

A11y and performance are not features that are either working or not; they are never finished and in my opinion, should be a category on their own.

This looks great, but it will take some getting used to. I expect that folks will want to add, for example, both a [Type] Accessibility (a11y) and a [Type] Bug label. But your reasoning above makes good sense and it will simplify things considerably.

In thinking more about it, there should never be an accessibility "enhancement", since the lack of accessibility is a bug. Standardizing everything under a single top-level label [Type] Accessibility (a11y) should work well.

@priethor priethor marked this pull request as ready for review July 25, 2023 11:26
@bph
Copy link
Contributor

bph commented Jul 25, 2023

In thinking more about it, there should never be an accessibility "enhancement", since the lack of accessibility is a bug. Standardizing everything under a single top-level label [Type] Accessibility (a11y) should work well.

Strongly agree with @ndiego and @priethor

Copy link
Contributor

@bph bph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will simplify the time release leads spent on curating the plugin release changelog tremendously.
LGTM

@afercia
Copy link
Contributor

afercia commented Jul 26, 2023

In thinking more about it, there should never be an accessibility "enhancement", since the lack of accessibility is a bug.

@ndiego thank you for raising this point. I'd totally agree and for some context I can confirm that historically in Core any a11y issue has always been considered a bug, at the point small a11y fixes used to be allowed even during beta.

@priethor
Copy link
Contributor Author

Thanks for the context and feedback! Given there is support towards treating Accessibility as a type of its own (or, at least, never an enhancement), I'll move forward and add the [Type] prefix to the Accessibility label for consistency before merging this PR. Always happy to roll back if needed! 🙂

@priethor priethor merged commit fbd0735 into trunk Jul 26, 2023
50 checks passed
@priethor priethor deleted the automation/enforce-pr-labels branch July 26, 2023 12:26
@github-actions github-actions bot added this to the Gutenberg 16.4 milestone Jul 26, 2023
@andrewserong
Copy link
Contributor

Just noticed this warning on a PR that had the [Type] Regression label, which comes up on PRs from time to time. Would it be worth adding that as one of the allowed types? If so, I can open a quick PR on Monday.

@priethor
Copy link
Contributor Author

Just noticed this warning on #52964 that had the [Type] Regression label, which comes up on PRs from time to time. Would it be worth adding that as one of the allowed types? If so, I can open a quick PR on Monday.

I didn't add it originally to the list because it is completely clear when to use bug vs. regression, and in terms of communication (changelog, release notes), both bugs and regressions are combined in the bugfixes bucket. However, I've just merged one last PR with follow-ups that enables the [Type] Regression label until further notice, given that it's there.

@andrewserong
Copy link
Contributor

Wonderful, thanks for adding it in @priethor! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Project Management Meta-issues related to project management of Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants