-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Workflows: Make 'Enforce labels on PRs' action less strict #54724
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to allow PRs to have the a11y label and another type of label, I don't think this is the best path. The reason for adding the label enforcer was twofold:
- To prevent PRs from not having any type label.
- To prevent PRs from having too many type labels.
If we want to allow more than one type label in some cases, we need to define which ones make sense and then make them not type labels; probably there are too many of them right now. For example, we could make the a11y label not a type label so that it's always paired with something else. On the other hand, a bug should never have a second type, like documentation, which is a pattern I've seen quite a few times 😅
|
I don't have a preference regarding the a11y label, but I think it was recently changed to a type. cc @alexstine, @afercia, @joedolson |
|
Accessibility is certainly a type change as some PRs only focus on accessibility. That's not to say that someone works on a bug fix or enhancement though and also patches an accessibility issue. I've been known to block PRs that way so stuff gets fixed. Best example of this would probably be accessibility and regression types. Sadly, it happens often enough. Tracking regressions is important and probably more important than the bug label itself. |
I agree with both sentences, but that doesn't mean the latter should be labeled as a11y from a project management (and reporting) perspective. To put an example with other types other than a11y, PRs that follow a single responsibility principle (most of them) have a primary type (e.g., enhancement or bugfix) but they also include other kinds of changes, like updating the documentation, refactoring, and adding e2e tests. However, these changes are inherent to the primary goal, but that doesn't mean the PR should be classified as a documentation change, a code quality improvement, or a testing change. I'm open to interpretations here as this is highly subjective, though! 🙂 Here's another possible path:
I know this might seem trivial for many folks (it's just labels, right?), but these are founding stones to build better automation and project tracking. Moreover, if we want to go monorepo sometime, we need more defined structures 😄 |
priethor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To summarize my comments above, I believe we need to clarify what types of tasks and labels we should have, as this change fixes one edge case but introduces more undesired changes that will impact project management and reporting. 🙇
|
In my opinion, the accessibility type label should be used for issues that are specifically focused on accessibility, whether they're adding an enhancement or fixing a bug. The Needs Accessibility Feedback label should be used for any other component that needs feedback. The distinction between an accessibility bug fix and an accessibility enhancement can be subtle, but it is a crucial distinction, so I think it would be good to be able to reflect that better in reporting. An accessibility bug fix is "we have an accessibility problem, and we need to fix it", where an accessibility enhancement is more "this is a change that would make the experience better for users, though they are able to use it now." Because accessibility crosses over with usability so much, those lines can be very blurry: it depends on just how burdensome the current flow is. To some degree, this is a question of how the project is going to treat accessibility as a whole. Do we distinguish between different types of accessibility issues, or should we simply have a list of Accessibility issues, and treat them as their own class of concern? E.g., Accessibility could no longer be paired with bugfix or enhancement; it's just Accessibility. That can work, as long as all accessibility issues are prioritized. Thinking in terms of workflows, what I want to be able to find, most of the time, is either "Accessibility issues targeted for 6.4 (whether they're enhancements or bugs)", "Accessibility enhancements (regardless of milestone)", and "Accessibility bugs (regardless of milestone)"; whatever labeling will help me get that information reported is pretty much OK for me. |
Is it daft to think that "Bug", "Enhancement", "Regression", "Breaking Change" should not be types? Generally speaking they are but it can make sense to look at them as qualifications instead. That way they can mix with any of the types and still convey their useful information. |
I agree it's not trivial and I do realize is not simple. However, there are a couple things that in my opinion need to be better considered. 1 2 The Core Trac for example is certainly a project management tool but it is also the history of the project. For example, at any moment, a contributor, or a project manager, or a lead, should be able to get data to make decisions easier. For example, say that as a project manager or as a lead I want to know how many Performance Regressions issues have been created from 6 months to 12 months ago. Right now, those are two 'Type' labels and issues can't use both of them. Also, on Trac we've always been able to manually create new keywords. Those are extremely helpful to group together a series of tickets having the same scope. For example: https://core.trac.wordpress.org/query?keywords=~title-attribute That keyword allows everybody to quickly retrieve all the tickets related to a specific topic. It also can be shared on other tickets or blog posts to point people on the work that has been done or that is still ongoing. It's history and documentation at the same time. I think we should really thing at lables also as a tool to facilitate decisional process and as the history of the project. I'm not sure a super 'strict' management of labels is ideal for that. There should be a little more flexibility, like on Trac the Components, Focuses, and Keywords allos for more flexibility. On GitHub there are labels and I'd think we should allow a more liberal useage of them, as long as they follow some established conventions. |
I totally agree, and agreeing on and establishing that convention is what I want to achieve in the long term! Automation, like the label enforcer, is only a way to make sure the convention is followed (similarly to how code conventions are checked); otherwise, it's naive to think all contributors will follow the convention. My intention has never been to imply labels equal to types of issues, but the opposite. I see labels as "scoped", so there are different buckets of labels that could match Components, Focuses, Keywords... and Types of tasks. In my mind, Type labels should be mutually exclusive in a PR, but there can be (and there are) multiple packages, for example. My question to you all would be, how does the accessibility team want a11y labels to be treated? As a type, as a focus? I honestly don't know what the right answer is, but strongly believe it should be belong to one bucket and consistent, so it's not treated sometimes as a type (replacing bugfix/enhancement labels) and others as a focus (coexisting with the bugfix/enhancement labels); otherwise it's not only confusing but hard to manage. For a more general conversation around standardizing labels in Gutenberg, I opened this discussion a while ago and would appreciate y'all's input. 🙏 |
|
Thanks @priethor. I think I already pointed out in some other discussion or issue that Accessibility is typically something that spans across different areas of a project. As such, we should be able to add the Accessibility label everywhere. WhethEr it's a Type or other, I don't have strong preferences. Accessibility in Core Trac used to be a component and was changed to Focus right for that reason. The re=organizazion of the Trac focuses dates early 2014. For more details and the reasoning about Accessibility as focus, see https://make.wordpress.org/core/2014/01/24/trac-focuses/ |
I think that's the root problem. It the Type label must be unique (not sure I fully understand why, but regardless...) then they should be very few and be used only for a unique type of data, e.g.: the bug type. |
Not having a unique type has proven difficult both for contributors, not knowing how to label PRs, and for folks contributing to project management, as it is common to find PRs with objectively wrong labels. The most clear example is finding PRs with, at the same time, Documentation and Bug to refer to a typo in a documentation 😅 . Unfortunately, it's easier to align all contributors on the guidelines with some guardrails in the form of automation, similar to how linters analyze the code and enforce some conventions. As Gutenberg grows and we try to automate more project management to help contributors, this becomes increasingly needed. I've opened an alternative approach to this PR here: #54941 |
|
Closing in favor of #54941. |
What?
When setting two labels for change type - [Type] {type}, I noticed that the action was producing a failure.
The PR makes the action less strict by changing the
modetominimum. This way, it will require at least one label from the list, but it won't fail when more are provided.Why?
Some changes can span across multiple type groups. Examples:
[Type] a11ychange can be a bug fix or an enhancement.[Type] Bugcan also introduce a breaking change in codeTesting Instructions
These changes are hard to test in the repo. But I double-checked this would be expected behavior in the action's tests.