improve protected branch to add whitelist support #2451

lunny · 2017-09-02T10:05:44Z

This PR will add a protected branch edit page to enable or disable protected branch. It also add support to whitelist users or whitelist teams so that special users could push to the branch.

lafriks · 2017-09-02T13:18:59Z

models/migrations/v39.go

+		if _, err := x.Exec("ALTER TABLE protected_branch DROP COLUMN can_push"); err != nil {
+			return fmt.Errorf("DROP COLUMN can_push: %v", err)
+		}
+	default:


Need also migration support for PostgreSQL and MSSQL

I think I have done that

Sorry didn't see, my mistake :)

lafriks · 2017-09-02T13:29:53Z

models/migrations/v39.go

+		if _, err := x.Exec("ALTER TABLE protected_branch DROP COLUMN can_push"); err != nil {
+			return fmt.Errorf("DROP COLUMN can_push: %v", err)
+		}
+	default:


Sorry didn't see, my mistake :)

ethantkoenig · 2017-09-02T23:42:28Z

routers/repo/setting_protected_branch.go

+	} else {
+		if protectBranch != nil {
+			if err := ctx.Repo.Repository.DeleteProtectedBranch(protectBranch.ID); err != nil {
+				ctx.Handle(500, "UpdateProtectBranch", err)


Wrong error message

ethantkoenig · 2017-09-02T23:44:42Z

options/locale/locale_en-US.ini

@@ -922,6 +922,16 @@ settings.protected_branch=Branch Protection
 settings.protected_branch_can_push=Allow push?
 settings.protected_branch_can_push_yes=You can push
 settings.protected_branch_can_push_no=You can not push
+settings.branch_protection = Branch Protection for <b>%s</b>
+settings.protect_this_branch = Protect this branch
+settings.protect_this_branch_desc = Disable force pushes and prevent from deletion.


I would suggest Disable force pushes and prevent deletion.

ethantkoenig · 2017-09-02T23:45:18Z

options/locale/locale_en-US.ini

+settings.branch_protection = Branch Protection for <b>%s</b>
+settings.protect_this_branch = Protect this branch
+settings.protect_this_branch_desc = Disable force pushes and prevent from deletion.
+settings.update_protect_branch_success = Change branch protect options successfully.


I would suggest Branch protect options changed successfully.

ethantkoenig · 2017-09-02T23:45:48Z

options/locale/locale_en-US.ini

+settings.protect_whitelist_committers_desc = Add people or teams to whitelist of direct push to this branch. Users in whitelist will bypass require pull request check.
+settings.protect_whitelist_users = Users who can push to this branch
+settings.protect_whitelist_search_users = Search users
+settings.protect_whitelist_teams = Teams for which members of them can push to this branch


I would suggest Teams whose members can push to this branch.

ethantkoenig · 2017-09-02T23:48:08Z

options/locale/locale_en-US.ini

+settings.protect_this_branch_desc = Disable force pushes and prevent from deletion.
+settings.update_protect_branch_success = Change branch protect options successfully.
+settings.protect_whitelist_committers = Whitelist who can push to this branch
+settings.protect_whitelist_committers_desc = Add people or teams to whitelist of direct push to this branch. Users in whitelist will bypass require pull request check.


I would suggest Add users or teams to this branch's whitelist. Whitelisted users bypass the typical push restrictions.

ethantkoenig · 2017-09-02T23:51:07Z

models/migrations/v39.go

+
+	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/setting"
+	"github.com/go-xorm/xorm"


Blank space

lafriks · 2017-09-04T06:02:08Z

LGTM

lunny · 2017-09-04T06:07:37Z

models/branches.go

+	if protectBranch.ID == 0 {
+		if _, err = x.Insert(protectBranch); err != nil {
+			return fmt.Errorf("Insert: %v", err)
+		}


should return nil

bkcsoft · 2017-09-04T11:51:23Z

@lunny From the screenshot, do we really need to differentiate between Users and Teams? Can't they be in the same list? (Gitea knows the difference internally anyhow)

lunny · 2017-09-04T11:53:07Z

@bkcsoft For a non-organization repository, we only have users. For an organization repository, we will have users and teams. I think both are needed.

bkcsoft · 2017-09-04T12:12:41Z

@lunny Right, the list it returns would be different, but I still don't understand why it needs to be 2 lists?

lunny · 2017-09-04T12:14:49Z

Two lists is easy to implement. 😄 We can optimist it later. We still need pull request control, status check and approval control.

lafriks · 2017-09-04T12:20:49Z

I also would like to see there single list instead of two but I agree that it can be implemented later

ethantkoenig · 2017-09-07T18:55:44Z

models/branches.go

-	} else if has {
-		return nil
-	}
+func (p Int64Slice) Len() int           { return len(p) }


Would it be possible to move these helpers for int64 slices to a util file? They don't have anything to do with branches.

ethantkoenig · 2017-09-07T19:00:22Z

models/org.go

@@ -577,6 +577,11 @@ func (org *User) getUserTeamIDs(e Engine, userID int64) ([]int64, error) {
 		Find(&teamIDs)
 }

+// TeamsHaveAccessToRepo returns all teamsthat have given access level to the repository.
+func (org *User) TeamsHaveAccessToRepo(repoID int64, mode AccessMode) ([]*Team, error) {


I'd suggest renaming to TeamsWithAccessToRepo(and similarly GetTeamsWithAccessToRepo)

ethantkoenig · 2017-09-07T19:01:24Z

models/repo.go

+}
+
+// getUsersWithAccesMode returns users that have at least given access mode to the repository.
+func (repo *Repository) getUsersWithAccesMode(e Engine, mode AccessMode) (_ []*User, err error) {


misspelled Access

lunny · 2017-09-12T11:58:02Z

will be part of #32

lunny · 2017-09-12T11:58:25Z

@ethantkoenig all done.

LanaYuan · 2017-09-14T06:22:32Z

my gitea version 1.2 the protect branch tool didn't work, but when i use version 1.1.3, it works well

lunny · 2017-09-14T06:30:01Z

@LanaYuan could you try on https://try.gitea.io

appleboy · 2017-09-14T07:30:34Z

LGTM

codecov-io · 2017-09-14T08:12:06Z

Codecov Report

Merging #2451 into master will decrease coverage by 0.08%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2451      +/-   ##
==========================================
- Coverage   27.83%   27.75%   -0.09%     
==========================================
  Files          83       83              
  Lines       16835    16886      +51     
==========================================
  Hits         4686     4686              
- Misses      11474    11525      +51     
  Partials      675      675

Impacted Files	Coverage Δ
models/repo.go	`13.53% <0%> (-0.2%)`	⬇️
modules/base/tool.go	`73.99% <0%> (-1.41%)`	⬇️
models/org_team.go	`52.74% <0%> (-1.43%)`	⬇️
models/org.go	`69.36% <0%> (-0.3%)`	⬇️
models/branches.go	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be3319b...f71a25a. Read the comment docs.

lunny added the backport/done All backports for this PR have been created label Sep 2, 2017

lunny added this to the 1.2.0 milestone Sep 2, 2017

lunny added type/enhancement An improvement of existing functionality and removed backport/done All backports for this PR have been created labels Sep 2, 2017

lunny modified the milestones: 1.3.0, 1.2.0 Sep 2, 2017

lafriks requested changes Sep 2, 2017

View reviewed changes

lafriks approved these changes Sep 2, 2017

View reviewed changes

ethantkoenig suggested changes Sep 2, 2017

View reviewed changes

ethantkoenig approved these changes Sep 3, 2017

View reviewed changes

tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Sep 4, 2017

lunny commented Sep 4, 2017

View reviewed changes

lunny force-pushed the lunny/improve_proteced_branch branch 2 times, most recently from 7cea815 to 1666907 Compare September 6, 2017 10:52

ethantkoenig suggested changes Sep 7, 2017

View reviewed changes

lunny force-pushed the lunny/improve_proteced_branch branch 2 times, most recently from 0ad42cc to bcadda5 Compare September 12, 2017 11:36

lunny mentioned this pull request Sep 12, 2017

Gitea hosted Gitea #1029

Open

20 tasks

ethantkoenig approved these changes Sep 12, 2017

View reviewed changes

improve protected branch to add whitelist support

769d918

lunny added 8 commits September 14, 2017 15:28

fix lint

75454c1

fix style check

14098fa

fix tests

cb1f1b7

fix description on UI and import

1a6f72a

fix test

231b024

bug fixed

d6d0151

fix tests and languages

3ee5d15

move isSliceInt64Eq to util pkg; improve function names & typo

f71a25a

lunny force-pushed the lunny/improve_proteced_branch branch from bcadda5 to f71a25a Compare September 14, 2017 07:28

appleboy approved these changes Sep 14, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 14, 2017

lunny merged commit 1739e84 into go-gitea:master Sep 14, 2017

lunny deleted the lunny/improve_proteced_branch branch September 14, 2017 08:16

daviian mentioned this pull request Sep 21, 2017

database issue after updating #2567

Closed

7 tasks

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve protected branch to add whitelist support #2451

improve protected branch to add whitelist support #2451

lunny commented Sep 2, 2017

lafriks Sep 2, 2017

lunny Sep 2, 2017

lafriks Sep 2, 2017

lafriks Sep 2, 2017

ethantkoenig Sep 2, 2017

lunny Sep 3, 2017

ethantkoenig Sep 2, 2017

lunny Sep 3, 2017

ethantkoenig Sep 2, 2017

lunny Sep 3, 2017

ethantkoenig Sep 2, 2017

lunny Sep 3, 2017

ethantkoenig Sep 2, 2017

lunny Sep 3, 2017

ethantkoenig Sep 2, 2017

lunny Sep 3, 2017

lafriks commented Sep 4, 2017

lunny Sep 4, 2017

bkcsoft commented Sep 4, 2017

lunny commented Sep 4, 2017 •

edited

Loading

bkcsoft commented Sep 4, 2017

lunny commented Sep 4, 2017 •

edited

Loading

lafriks commented Sep 4, 2017

ethantkoenig Sep 7, 2017

ethantkoenig Sep 7, 2017

ethantkoenig Sep 7, 2017

lunny commented Sep 12, 2017

lunny commented Sep 12, 2017

LanaYuan commented Sep 14, 2017

lunny commented Sep 14, 2017

appleboy commented Sep 14, 2017

codecov-io commented Sep 14, 2017

improve protected branch to add whitelist support #2451

improve protected branch to add whitelist support #2451

Conversation

lunny commented Sep 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Sep 4, 2017

Choose a reason for hiding this comment

bkcsoft commented Sep 4, 2017

lunny commented Sep 4, 2017 • edited Loading

bkcsoft commented Sep 4, 2017

lunny commented Sep 4, 2017 • edited Loading

lafriks commented Sep 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Sep 12, 2017

lunny commented Sep 12, 2017

LanaYuan commented Sep 14, 2017

lunny commented Sep 14, 2017

appleboy commented Sep 14, 2017

codecov-io commented Sep 14, 2017

Codecov Report

lunny commented Sep 4, 2017 •

edited

Loading

lunny commented Sep 4, 2017 •

edited

Loading