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.

Sign up for GitHub

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

Jump to bottom

Add codeowners feature #24910

Merged
merged 21 commits into from
Jun 8, 2023
Merged

Add codeowners feature #24910

merged 21 commits into from
Jun 8, 2023

Conversation

cl-bvl
Copy link
Contributor

@cl-bvl cl-bvl commented May 24, 2023 •
edited
Loading

Hello.
This PR adds a github like configuration for the CODEOWNERS file.

Resolves: #10161

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 24, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 24, 2023
@yardenshoham yardenshoham added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label May 24, 2023
@cl-bvl cl-bvl marked this pull request as draft May 24, 2023 12:24
@cl-bvl cl-bvl marked this pull request as ready for review May 24, 2023 14:58
@cl-bvl @techknowlogick
Update modules/git/repo_show.go
b7ec465 
Co-authored-by: techknowlogick <matti@mdranta.net>
a1012112796
a1012112796 reviewed May 25, 2023
View reviewed changes
services/pull/pull.go Outdated Show resolved Hide resolved
services/pull/pull.go Outdated Show resolved Hide resolved
modules/git/repo_show.go Outdated Show resolved Hide resolved
services/pull/pull.go Outdated Show resolved Hide resolved
@cl-bvl cl-bvl requested review from lunny and KN4CK3R May 28, 2023 14:42
wxiaoguang
wxiaoguang reviewed May 29, 2023
View reviewed changes
services/pull/pull.go Outdated
Users []*user_model.User
}

func parseCodeOwnersLine(ctx context.Context, tokens []string) (*CodeOwnerRule, []string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it needs some tests to cover the code, to make sure the logic won't be broken by future changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test for file parser

lunny
lunny reviewed May 29, 2023
View reviewed changes
services/pull/check.go Outdated Show resolved Hide resolved
lunny
lunny reviewed May 29, 2023
View reviewed changes
services/pull/pull.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented May 29, 2023

  • We need to check who can create the CodeOwner file. As GH's docs, admin permission is required.
  • Reviewers could also be team
  • When to send review requests to reviewers. A draft doesn't need to be reviewed immediately. Only a normal pull request or a pull request converted from a draft need to notify reviewers.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 3, 2023
@lunny lunny added this to the 1.20.0 milestone Jun 3, 2023
@lunny
Copy link
Member

lunny commented Jun 4, 2023

Sent 7ddd654 to fix th lint

@delvh delvh added the reviewed/prioritize-merge PR is in the merge queue. Merge as soon as possible, i.e. as edits by maintainers are not enabled label Jun 4, 2023
@silverwind
Copy link
Member

silverwind commented Jun 7, 2023 •
edited
Loading

@cl-bvl can you allow push access to your branch? Otherwise this will be hard to merge on UI at least with our branch protection rules in place.

@wxiaoguang
Copy link
Contributor

I guess that's GitHub's limitation, GitHub doesn't work that way. Since this PR is from a org (WinnerSoftLab), so no "Allow maintainer edit". Only the author can update the PR unless someone is added as the fork's collaborator.

@silverwind
Copy link
Member

silverwind commented Jun 7, 2023 •
edited
Loading

So effectively, this can likely only be merged manually on the CLI.

In any case, lint needs to be fixed first.

@denyskon
Copy link
Member

denyskon commented Jun 7, 2023

So effectively, this can likely only be merged manually on the CLI.

In any case, lint needs to be fixed first.

What about #24910 (comment) ?

@delvh delvh removed this from the 1.20.0 milestone Jun 7, 2023
@lunny
Copy link
Member

lunny commented Jun 8, 2023

I guess that's GitHub's limitation, GitHub doesn't work that way. Since this PR is from a org (WinnerSoftLab), so no "Allow maintainer edit". Only the author can update the PR unless someone is added as the fork's collaborator.

I in fact sent a PR to that fork repository, they just need to review and merge it.

@silverwind silverwind enabled auto-merge (squash) June 8, 2023 08:07
@silverwind silverwind added backport/v1.20 This PR should be backported to Gitea 1.20 and removed backport/v1.20 This PR should be backported to Gitea 1.20 labels Jun 8, 2023
@silverwind
Copy link
Member

silverwind commented Jun 8, 2023 •
edited
Loading

Branch needs manual merge as we can not push merges to it.

@silverwind silverwind disabled auto-merge June 8, 2023 08:55
@lunny lunny merged commit 3bdd480 into go-gitea:main Jun 8, 2023
@GiteaBot GiteaBot added this to the 1.21.0 milestone Jun 8, 2023
@GiteaBot GiteaBot removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. reviewed/prioritize-merge PR is in the merge queue. Merge as soon as possible, i.e. as edits by maintainers are not enabled labels Jun 8, 2023
silverwind added a commit to silverwind/gitea that referenced this pull request Jun 8, 2023
@silverwind
Merge branch 'main' into buttonfix
757ca22 
* main:
  Modify OAuth login ui and fix display name, iconurl related logic (go-gitea#25030)
  Fix open redirect check for more cases (go-gitea#25143)
  Update js dependencies (go-gitea#25137)
  Remove duplicated functions when deleting a branch (go-gitea#25128)
  Add codeowners feature (go-gitea#24910)
  Fix strange UI behavior of cancelling dismiss review modal (go-gitea#25133)
  Fix `MilestoneIDs` when querying issues (go-gitea#25125)
  Fix incorrect git ignore rule and add missing license files (go-gitea#25135)
  Change branch name from master to main in some documents' links (go-gitea#25126)
  Remove incorrect element ID on "post-install" page (go-gitea#25104)
  [skip ci] Updated translations via Crowdin
  Improve notification icon and navbar  (go-gitea#25111)
  fix swagger documentation for multiple files API endpoint (go-gitea#25110)
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 9, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
482bdb7 
* upstream/main:
  [skip ci] Updated translations via Crowdin
  Modify OAuth login ui and fix display name, iconurl related logic (go-gitea#25030)
  Fix open redirect check for more cases (go-gitea#25143)
  Update js dependencies (go-gitea#25137)
  Remove duplicated functions when deleting a branch (go-gitea#25128)
  Add codeowners feature (go-gitea#24910)
  Fix strange UI behavior of cancelling dismiss review modal (go-gitea#25133)
  Fix `MilestoneIDs` when querying issues (go-gitea#25125)
  Fix incorrect git ignore rule and add missing license files (go-gitea#25135)
  Change branch name from master to main in some documents' links (go-gitea#25126)
  Remove incorrect element ID on "post-install" page (go-gitea#25104)
  [skip ci] Updated translations via Crowdin
  Improve notification icon and navbar  (go-gitea#25111)
  fix swagger documentation for multiple files API endpoint (go-gitea#25110)
  Fix webauthn regression and improve code (go-gitea#25113)
  Add details summary for vertical menus in settings to allow toggling (go-gitea#25098)
  Fix 500 error caused by notifications without an issue such as repo transfers (go-gitea#25101)
@tacerus
Copy link

tacerus commented Aug 8, 2023 •
edited
Loading

Hello,

I'm experimenting with this feature on a build of Gitea as of commit 4fc4f6e. Unfortunately I cannot seem to make it work, adding a CODEOWNERS file to the root of my repository with the content:

directory/.*\.yaml @crameleon-test2

Does not give the user crameleon-test2 the option to approve or merge a pull request containing changes to a file matching this regex pattern.

Going through the changes in this patch, there does not seem to be anything else that needs to be configured.
Does someone have an idea on what I'm missing? Would appreciate any help!

@KN4CK3R
Copy link
Member

KN4CK3R commented Aug 9, 2023

I think this PR just adds a "request for review" and not the ability to approve or merge (?)

@tacerus
Copy link

tacerus commented Aug 9, 2023

I see, thank you.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@lunny lunny lunny approved these changes

@techknowlogick techknowlogick Awaiting requested review from techknowlogick

@a1012112796 a1012112796 Awaiting requested review from a1012112796

@KN4CK3R KN4CK3R Awaiting requested review from KN4CK3R

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.

support Github-like CODEOWNERS?