Skip to content

Stop auto labelers because there are too many inacculate behaviours to make confusing #27439

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

Closed
wants to merge 1 commit into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Oct 4, 2023

It's very difficult to get to know the aim of PR creator according changed files. A file maybe changed project files but the PR maybe a bugfix/a refactor/a comment added and etc.

Revert #26962

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 4, 2023
@silverwind
Copy link
Member

silverwind commented Oct 4, 2023

If our intend is to capture PR intend in the labels, then yes I agree, the labeler is unsuited.

@silverwind
Copy link
Member

silverwind commented Oct 4, 2023

Labeler is only suitable for "subsystem" style labels, like ui or build. Maybe keep those uncontested cases?

@silverwind
Copy link
Member

Overall I think the labeler is more beneficial than it hurts. Take a look at recent PRs, they do have good labels.

Maybe we should disable the sync-labels option so that it never removes labels? I think some of the issues come from this option.

@lunny
Copy link
Member Author

lunny commented Oct 5, 2023

i.e. The auto labelers for #23894 is totally wrong and annoying. And I also encounter many times like this.

@silverwind
Copy link
Member

silverwind commented Oct 5, 2023

It correctly labeled what we told it to. Big refactors like that are likely to trigger many labels. I think we should probably just remove these labels from the config: theme/package-registry, kind/api, kind/cli, kind/lint and set sync-labels: false.

@wxiaoguang
Copy link
Contributor

It's a kind of laziness to let a bot process the unclear labels.

I do not think the labeler is really useful at the moment. The mergers should should pay enough attention to the PR itself (title, description, code change) and the labels, instead of just clicking the "merge" button.

And the label system is not well-designed, for example: what is kind/ui kind/ux kind/usability javascript?

When there is a problem, it's not right to say "oh, just add a bot to deal with it, nobody would really spend time on it".

@wxiaoguang
Copy link
Contributor

If anyone really spent time on the label system, really ever maintained the labels, and then introduce the labeler carefully, then I could agree with them.

@silverwind
Copy link
Member

silverwind commented Oct 5, 2023

I can guarantee that if we remove the labeler, many PRs will merge unlabeled because many do not care about those labels in reality. I'm fine with merging unlabeled PRs too, I don't need them. But the changelog tool might.

Another option might be to require a prefix on commit message to categorize the PRs and remove most if not all of the labels.

@wxiaoguang
Copy link
Contributor

I can guarantee that if we remove the labeler, many PRs will merge unlabeled because many do not care about those labels in reality.

Default/no-op is better than wrong.

But the changelog tool might.

I can understand such requirement. IMO the solution for such "label problem" should start from the changelog tool.

Define which kinds of labels are really needed and necessary first, maintain the current labels, remove dead ones, clarify how to use the labels correctly, then fine tune the labeler bot.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 5, 2023

Another option might be to require a prefix on commit message to categorize the PRs and remove most if not all of the labels.

That's somewhat related. One of the problems is that the "changelog" rules are not clear. For example, why there is a separate "API" section? A change should be either a feature/enhancement/bugfix/.../misc. If a PR does a bugfix(enhancment) for both web and api, why it should appear in the API section?

The historical unclear rules are the root problem.

@silverwind
Copy link
Member

silverwind commented Oct 5, 2023

I wouldn't mind if we just set a optional single category label manually like feature,enhancement,fix,refactor.

If we retain subsystem labels like ui, those should stay in the labeler imho.

@denyskon
Copy link
Member

denyskon commented Oct 5, 2023

I agree that we need to rework our label system before we start applying labels automatically.

I would suggest:

  1. split PR and issue labels
  2. for issues, keep it simple and only separate by kind (proposal, bugreport, question, misc) and status (needs feedback, confirmed, in implementation)
  3. decide what PR labels are for (which information are they supposed to provide?)
    I can think of the following categories:
  • section - which part(s) of the codebase does it affect: templates, web_src, backend, api
  • kind - what does this PR do: feature, bugfix, enhancement, refactoring, docs
  • topic - which functionality does it affect: package-registry, administration, actions, wiki, issues, repos, authentication, ui
  • review status: wip, blocked, ready to merge, in review, pending, urgent
  • MAYBE complexity, but not according to the useless size bot but according to the author

I think that would make labels more logical and useful for everyone. And the section part is the only one we can automatically apply according to the files changed.

@silverwind silverwind mentioned this pull request Oct 7, 2023
@silverwind
Copy link
Member

Alternative PR: #27502

silverwind added a commit that referenced this pull request Oct 8, 2023
Alternative to #27439. Removes a
few spammy labels, and disables `sync-labels` which make it never remove
labels (which is default behaviour).
@silverwind
Copy link
Member

#27502 was merged, pending further adjustments.

@silverwind silverwind closed this Oct 8, 2023
@lunny lunny deleted the lunny/stop_auto_label branch October 9, 2023 01:01
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/miscellaneous
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants