github-starred/gitea
2
0
Fork 0
mirror of https://github.com/go-gitea/gitea.git synced 2026-03-12 10:39:38 -05:00
Code Issues 2.6k Packages Projects Releases 100 Wiki Activity

RFC: Refactoring the review system #4311

New Issue
Open
opened 2025-11-02 05:45:33 -06:00 by GiteaMirror · 16 comments
GiteaMirror commented 2025-11-02 05:45:33 -06:00
Owner
Copy Link

Originally created by @3l73 on GitHub (Nov 14, 2019).

RFC: Refocturing the review system

This issue I an request for comment and about changing the whole review system.

A colleague of mine recommend to introduce rebuild the review system as a new feature in it's own. This allows use to use current infrastructure and setup a structure in depend from the current code.

Since I will follow his recommendation, some terms will differ from what you currently know.

Let's talk about audit's.
A pull request can contain one or more of it. An audit is done by an author and contains a list of audit tasks.
The audit it self can reject or agree the pull request.

An audit task can be connect to a change done by the developer.
It contains comments and a status:
Done? True/False

As an audit must not connect to an technical change, this allows the developer starting general diskussions about the code.

If a task is completed, the author of the audit mark it as completed or done.
The Administrator, can mark all task as done and overwrite the authors decision.

If an audit contains task that are not marked as done, it is not fulfilled.
As a consequence the pull request could not merged into the selected branch.

An audit can created by a human person or a bot/hook.

For a example a bot could check the "code smell" and block the pull request.
In this case the bot will update the audit after all pull request.

The administrator can decide what audit have to e fulfilled in order to merge, and how many audit's have to be done and completed.

As you see, additional information is required, in order to decide if an audit task is required or optional.

What additional should be checked?

  • if a code changed, removed or inside another file, an audit task has to be updated

What is your opinion?
Have you ideas what should be extended?

Features that need to be done

  • Pull Request should not be able to merged, if one ore more audits are denied

Features that are nice to have

  • TBD

Features to be discussed

  • Administrator allowed to override status of an audit
Originally created by @3l73 on GitHub (Nov 14, 2019). RFC: Refocturing the review system This issue I an request for comment and about changing the whole review system. A colleague of mine recommend to introduce rebuild the review system as a new feature in it's own. This allows use to use current infrastructure and setup a structure in depend from the current code. Since I will follow his recommendation, some terms will differ from what you currently know. Let's talk about audit's. A pull request can contain one or more of it. An audit is done by an author and contains a list of audit tasks. The audit it self can reject or agree the pull request. An audit task can be connect to a change done by the developer. It contains comments and a status: Done? True/False As an audit must not connect to an technical change, this allows the developer starting general diskussions about the code. If a task is completed, the author of the audit mark it as completed or done. The Administrator, can mark all task as done and overwrite the authors decision. If an audit contains task that are not marked as done, it is not fulfilled. As a consequence the pull request could not merged into the selected branch. An audit can created by a human person or a bot/hook. For a example a bot could check the "code smell" and block the pull request. In this case the bot will update the audit after all pull request. The administrator can decide what audit have to e fulfilled in order to merge, and how many audit's have to be done and completed. As you see, additional information is required, in order to decide if an audit task is required or optional. What additional should be checked? - if a code changed, removed or inside another file, an audit task has to be updated What is your opinion? Have you ideas what should be extended? --- ### Features that need to be done - Pull Request should not be able to merged, if one ore more audits are denied ### Features that are nice to have - TBD ### Features to be discussed - Administrator allowed to override status of an audit
GiteaMirror added the type/proposal label 2025-11-02 05:45:33 -06:00
GiteaMirror commented 2025-11-02 05:45:33 -06:00
Author
Owner
Copy Link

@saitho commented on GitHub (Nov 14, 2019):

cc @jonasfranz

@saitho commented on GitHub (Nov 14, 2019): cc @jonasfranz
GiteaMirror commented 2025-11-02 05:45:34 -06:00
Author
Owner
Copy Link

@jonasfranz commented on GitHub (Nov 14, 2019):

What's the difference between the current review system and the audits? If I understood correctly, you want to add a feature to mark change requests as done?

@jonasfranz commented on GitHub (Nov 14, 2019): What's the difference between the current review system and the audits? If I understood correctly, you want to add a feature to mark change requests as done?
GiteaMirror commented 2025-11-02 05:45:34 -06:00
Author
Owner
Copy Link

@3l73 commented on GitHub (Nov 14, 2019):

There are some differences:

  1. The internal structure
    1. You can add task that have to be fulfilled even without an connection to code, that is currently not possible
    2. An audit has a status that said, fulfilled or still something not done
    3. A change has to be accepted
  2. An audit task has a status, comment are connected to the audit task and a single entity. Currently it is confusing how it handled
  3. Easy to understand. If audits contains task that are not fulfilled... Audit is not accepted
  4. Currently if someone change the code, what the reviewer written to the code is in some way "lost"
  5. The author of the audit has to accept the change

I had a look at the source of the current system, so most of the points are more connected to the internal aspect.

The system not really change, but allows some interesting things.

Call it an audit instead of a review comes from a discussion with a colleague.

His thoughts goes into direction to introduce the feature and switch it at a point when it is functional

@3l73 commented on GitHub (Nov 14, 2019): There are some differences: 1. The internal structure 1. You can add task that have to be fulfilled even without an connection to code, that is currently not possible 2. An audit has a status that said, fulfilled or still something not done 3. A change has to be accepted 2. An audit task has a status, comment are connected to the audit task and a single entity. Currently it is confusing how it handled 3. Easy to understand. If audits contains task that are not fulfilled... Audit is not accepted 4. Currently if someone change the code, what the reviewer written to the code is in some way "lost" 5. The author of the audit has to accept the change I had a look at the source of the current system, so most of the points are more connected to the internal aspect. The system not really change, but allows some interesting things. Call it an audit instead of a review comes from a discussion with a colleague. His thoughts goes into direction to introduce the feature and switch it at a point when it is functional
GiteaMirror commented 2025-11-02 05:45:34 -06:00
Author
Owner
Copy Link

@davidsvantesson commented on GitHub (Nov 14, 2019):

I work in an industry with a lot of quality standards, so I think I get some of the points you are making. Regarding the naming (review, audit or something else) I think that has very little with the tool to do, you can call it what you like.

Some comments:

  • You can use pull request templates to add e.g. default checklists
  • With a tool like LGTM that is used here (but more advanced rules), you can set up specific rules for what is required to merge.
  • I think the approved / request changes scheme is quite good in general, but might be limiting for some people and cases.

Is this issue inspired by Gerrit? (A tool I dislike but for other reasons). They have something called review labels and you can add custom review labels. It allows to set different values depending on label and you can configure different rules for disabling merge. Could that fulfill what you are looking for?

4\. Currently if someone change the code, what the reviewer written to the code is in some way "lost"

The review system needs to be improved, for sure.

@davidsvantesson commented on GitHub (Nov 14, 2019): I work in an industry with a lot of quality standards, so I think I get some of the points you are making. Regarding the naming (review, audit or something else) I think that has very little with the tool to do, you can call it what you like. Some comments: - You can use pull request templates to add e.g. default checklists - With a tool like LGTM that is used here (but more advanced rules), you can set up specific rules for what is required to merge. - I think the approved / request changes scheme is quite good in general, but might be limiting for some people and cases. Is this issue inspired by Gerrit? (A tool I dislike but for other reasons). They have something called [review labels ](https://www.gerritcodereview.com/config-labels.html)and you can add custom review labels. It allows to set different values depending on label and you can configure different rules for disabling merge. Could that fulfill what you are looking for? > 4\. Currently if someone change the code, what the reviewer written to the code is in some way "lost" The review system needs to be improved, for sure.
GiteaMirror commented 2025-11-02 05:45:34 -06:00
Author
Owner
Copy Link

@3l73 commented on GitHub (Nov 15, 2019):

The name audit was used as a placeholder.
If you call it review it is fine too. As you say, it is just a word.

Since I have no experience with Gerrit, I can say it is not inspired by it.

Template can be an option.
There were some situations, that leads a colleague and me to place a comment and start general discussions.
I am personally not a friend having a discussion over a review tool. A direct talk is the most best way in this cases. Though there are times where it is not possible to have a personal discussion, so leave a comment and block the merge would be a nice feature.

Currently it is possible to merge a pull request even if there is a denied review.
In some cases this is good if a reviewer is not possible to agree because of vacation or other circumstances.
A solution could be that the administrator accept the review or he can assing it to another developer who can mark the other review as accepted.

In the end a pull request should not be merged if some reviews are not accepted.

What kind of regulations should applied on a project should decided by the owner or admin of the project.

@3l73 commented on GitHub (Nov 15, 2019): The name audit was used as a placeholder. If you call it review it is fine too. As you say, it is just a word. Since I have no experience with Gerrit, I can say it is not inspired by it. Template can be an option. There were some situations, that leads a colleague and me to place a comment and start general discussions. I am personally not a friend having a discussion over a review tool. A direct talk is the most best way in this cases. Though there are times where it is not possible to have a personal discussion, so leave a comment and block the merge would be a nice feature. Currently it is possible to merge a pull request even if there is a denied review. In some cases this is good if a reviewer is not possible to agree because of vacation or other circumstances. A solution could be that the administrator accept the review or he can assing it to another developer who can mark the other review as accepted. In the end a pull request should not be merged if some reviews are not accepted. What kind of regulations should applied on a project should decided by the owner or admin of the project.
GiteaMirror commented 2025-11-02 05:45:34 -06:00
Author
Owner
Copy Link

@davidsvantesson commented on GitHub (Nov 15, 2019):

Currently it is possible to merge a pull request even if there is a denied review.

Apparently. I don't think that is good behavior, should at least be a setting.

@davidsvantesson commented on GitHub (Nov 15, 2019): > Currently it is possible to merge a pull request even if there is a denied review. Apparently. I don't think that is good behavior, should at least be a setting.
GiteaMirror commented 2025-11-02 05:45:35 -06:00
Author
Owner
Copy Link

@3l73 commented on GitHub (Nov 18, 2019):

I would collect all ideas at the top if this issue, so that they are located at on place.

@3l73 commented on GitHub (Nov 18, 2019): I would collect all ideas at the top if this issue, so that they are located at on place.
GiteaMirror commented 2025-11-02 05:45:35 -06:00
Author
Owner
Copy Link

@gerdemann commented on GitHub (Jan 16, 2020):

I would definitely have two more features that I miss a lot at the moment:

  1. comments on the code in the individual commits (https://github.com/go-gitea/gitea/issues/4898)
  2. automatically add default reviewers when creating a PR
@gerdemann commented on GitHub (Jan 16, 2020): I would definitely have two more features that I miss a lot at the moment: 1. comments on the code in the individual commits (https://github.com/go-gitea/gitea/issues/4898) 2. automatically add default reviewers when creating a PR
GiteaMirror commented 2025-11-02 05:45:35 -06:00
Author
Owner
Copy Link

@davidsvantesson commented on GitHub (Jan 16, 2020):

Now (coming release) there is a setting to block merge if there are rejected reviews.

@davidsvantesson commented on GitHub (Jan 16, 2020): Now (coming release) there is a setting to block merge if there are rejected reviews.
GiteaMirror commented 2025-11-02 05:45:35 -06:00
Author
Owner
Copy Link

@3l73 commented on GitHub (Jan 17, 2020):

Now (coming release) there is a setting to block merge if there are rejected reviews.

I like that feature.
May be we need a role to allow override a rejected Pull-Request in case the developer is not able to agree the Pull-Request (illness, vacation or what ever).

By the way, I miss the "Delete branch after merge" feature.

@3l73 commented on GitHub (Jan 17, 2020): > Now (coming release) there is a setting to block merge if there are rejected reviews. I like that feature. May be we need a role to allow override a rejected Pull-Request in case the developer is not able to agree the Pull-Request (illness, vacation or what ever). By the way, I miss the "Delete branch after merge" feature.
GiteaMirror commented 2025-11-02 05:45:36 -06:00
Author
Owner
Copy Link

@6543 commented on GitHub (Jan 18, 2020):

@3l73 to the new feature RepoAdmin is able to merge eafen review is not complete or status checks failed ...

@6543 commented on GitHub (Jan 18, 2020): @3l73 to the new feature RepoAdmin is able to merge eafen review is not complete or status checks failed ...
GiteaMirror commented 2025-11-02 05:45:36 -06:00
Author
Owner
Copy Link

@6543 commented on GitHub (Jan 18, 2020):

@3l73 and for the other things ... the easiest way is a ticket system trigerd by webhook witch set status check to false until tasks are completed

@6543 commented on GitHub (Jan 18, 2020): @3l73 and for the other things ... the easiest way is a ticket system trigerd by webhook witch set status check to false until tasks are completed
GiteaMirror commented 2025-11-02 05:45:36 -06:00
Author
Owner
Copy Link

@davidsvantesson commented on GitHub (Jan 18, 2020):

@6543 How would you do the ticket system?

Maybe there should be a possibility to make issue comments editable by other users (currently only writer of comment and repo admin can edit).

@davidsvantesson commented on GitHub (Jan 18, 2020): @6543 How would you do the ticket system? Maybe there should be a possibility to make issue comments editable by other users (currently only writer of comment and repo admin can edit).
GiteaMirror commented 2025-11-02 05:45:36 -06:00
Author
Owner
Copy Link

@6543 commented on GitHub (Jan 18, 2020):

@davidsvantesson

Maybe there should be a possibility to make issue comments editable by other users

dont think so ... could be abused

ticket system

I dont know a lot of ticket systems and its integration ... for example zammad has webhooks ...
And like CI status use tiket status ...

@6543 commented on GitHub (Jan 18, 2020): @davidsvantesson > Maybe there should be a possibility to make issue comments editable by other users dont think so ... could be abused > ticket system I dont know a lot of ticket systems and its integration ... for example zammad has webhooks ... And like CI status use tiket status ...
GiteaMirror commented 2025-11-02 05:45:36 -06:00
Author
Owner
Copy Link

@davidsvantesson commented on GitHub (Jan 18, 2020):

I don't mean anyone can edit anytime, there must be some way to set that.
The problem is today the issue or pull creator makes the first post with maybe a list or something, and noone else can edit that. There could for example be a way to set "make editable by others", so if you have a review protocol other persons can edit while the review procedes.

@davidsvantesson commented on GitHub (Jan 18, 2020): I don't mean anyone can edit anytime, there must be some way to set that. The problem is today the issue or pull creator makes the first post with maybe a list or something, and noone else can edit that. There could for example be a way to set "make editable by others", so if you have a review protocol other persons can edit while the review procedes.
GiteaMirror commented 2025-11-02 05:45:37 -06:00
Author
Owner
Copy Link

@6543 commented on GitHub (Jan 18, 2020):

@davidsvantesson like githubs [ ] Allow Maintainer to edit option on pulls?

@6543 commented on GitHub (Jan 18, 2020): @davidsvantesson like githubs `[ ] Allow Maintainer to edit` option on pulls?
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
$20
$250
$50
$500
backport/done
💎 Bounty
docs-update-needed
good first issue
hacktoberfest
issue/bounty
issue/confirmed
issue/critical
issue/duplicate
issue/needs-feedback
issue/not-a-bug
issue/regression
issue/stale
issue/workaround
lgtm/need 2
modifies/api
modifies/translation
outdated/backport/v1.18
outdated/theme/markdown
outdated/theme/timetracker
performance/bigrepo
performance/cpu
performance/memory
performance/speed
pr/breaking
proposal/accepted
proposal/rejected
pr/wip
pull-request
Mirrored from GitHub Pull Request
 reviewed/wontfix
💰 Rewarded
skip-changelog
status/blocked
topic/accessibility
topic/api
topic/authentication
topic/build
topic/code-linting
topic/commit-signing
topic/content-rendering
topic/deployment
topic/distribution
topic/federation
topic/gitea-actions
topic/issues
topic/lfs
topic/mobile
topic/moderation
topic/packages
topic/pr
topic/projects
topic/repo
topic/repo-migration
topic/security
topic/theme
topic/ui
topic/ui-interaction
topic/ux
topic/webhooks
topic/wiki
type/bug
type/deprecation
type/docs
type/enhancement
type/feature
type/miscellaneous
type/proposal
type/question
type/refactoring
type/summary
type/testing
type/upstream
No Label type/proposal
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/gitea#4311
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?