Protected branches system #339
Conversation
|
Tracking issue is #32 |
tboerger
added
lgtm/need 2
cmd/update.go
Outdated
|
|
||
| "github.com/urfave/cli" | ||
|
|
||
| "code.gitea.io/gitea/models" | ||
| "code.gitea.io/gitea/modules/log" | ||
| "code.gitea.io/gitea/modules/setting" | ||
| "github.com/gogits/git-module" |
models/protected_branch.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| PROTECTED_BRANCH_USER_ID = "__UER_ID" |
models/protected_branch.go
Outdated
| @@ -0,0 +1,138 @@ | |||
| // Copyright 2014 The Gogs Authors. All rights reserved. | |||
There was a problem hiding this comment.
+// Copyright 2016 The Gitea Authors. All rights reserved.
// Copyright 2014 The Gogs Authors. All rights reserved.
|
Please also take your time to document all new methods and functions. |
tboerger
added
the
lgtm/need 2
|
@denji please on next squash-rebase change the issue referenc from gogs/gogs#3921 to #32 (gitea issue) |
conf/locale/locale_de-DE.ini
Outdated
| @@ -748,6 +748,17 @@ settings.add_key_success=Der Deploy-Schlüssel '%s' wurde erfolgreich hinzugefü | |||
| settings.deploy_key_deletion=Deploy-Schlüssel löschen | |||
| settings.deploy_key_deletion_desc=Nach dem Löschen dieses Deploy-Schlüssels werden entsprechende Zugriffe auf dieses Repository nicht mehr möglich sein. Möchten Sie wirklich fortfahren? | |||
| settings.deploy_key_deletion_success=Deploy-Schlüssel wurde erfolgreich gelöscht! | |||
| protected_branch=Schutz-Filialen | |||
There was a problem hiding this comment.
Please don't use Google translate, these translations doesn't make sense at all
conf/locale/locale_en-US.ini
Outdated
| @@ -754,6 +754,17 @@ settings.add_key_success = New deploy key '%s' has been added successfully! | |||
| settings.deploy_key_deletion = Delete Deploy Key | |||
| settings.deploy_key_deletion_desc = Deleting this deploy key will remove all related accesses for this repository. Do you want to continue? | |||
| settings.deploy_key_deletion_success = Deploy key has been deleted successfully! | |||
| protected_branch=Protection branches | |||
conf/locale/locale_en-US.ini
Outdated
| settings.protected_branch_can_push=Allow push? | ||
| settings.protected_branch_can_push_yes=You can push | ||
| settings.protected_branch_can_push_no=You can't push | ||
| settings.add_protected_branch=Unprotection branch |
conf/locale/locale_en-US.ini
Outdated
| settings.protected_branch_can_push_yes=You can push | ||
| settings.protected_branch_can_push_no=You can't push | ||
| settings.add_protected_branch=Unprotection branch | ||
| settings.delete_protected_branch=Protection branch |
conf/locale/locale_fi-FI.ini
Outdated
| @@ -748,6 +748,17 @@ settings.add_key_success=Uusi deploy avain '%s' on lisätty onnistuneesti! | |||
| settings.deploy_key_deletion=Poista deploy avain | |||
| settings.deploy_key_deletion_desc=Deploy avaimen poistaminen poistaa myös kaikki liitetyt käyttötiedot tästä reposta. Haluatko jatkaa? | |||
| settings.deploy_key_deletion_success=Deploy avain on poistettu onnistuneesti! | |||
| protected_branch=Suojaa oksat | |||
conf/locale/locale_sr-SP.ini
Outdated
| @@ -748,6 +748,17 @@ settings.add_key_success=Нови кључ распоређивање '%s' је | |||
| settings.deploy_key_deletion=Уклони кључ распоређивањa | |||
| settings.deploy_key_deletion_desc=Брисање овог кључа за распоређивање ће довести до укидање приступ на овом спремишту. Да ли желите да наставите? | |||
| settings.deploy_key_deletion_success=Кључ за распоређивање је успешно обрисан! | |||
| protected_branch=Заштита грана | |||
conf/locale/locale_sv-SE.ini
Outdated
| @@ -748,6 +748,17 @@ settings.add_key_success=Den nya driftsättningsnyckeln '%s' har lagts till! | |||
| settings.deploy_key_deletion=Ta bort distribueringsnyckel | |||
| settings.deploy_key_deletion_desc=Borttagning av detta distributionsnyckel kommer att ta bort all relaterad åtkomst till det här repot. Vill du fortsätta? | |||
| settings.deploy_key_deletion_success=Distributionsnyckeln har tagits bort! | |||
| protected_branch=Skydd grenar | |||
conf/locale/locale_tr-TR.ini
Outdated
| @@ -748,6 +748,17 @@ settings.add_key_success=Yeni dağıtım anahtarı '%s' başarıyla eklendi! | |||
| settings.deploy_key_deletion=Dağıtım Anahtarını Sil | |||
| settings.deploy_key_deletion_desc=Bu dağıtım anahtarını silerseniz bu depoya ilişkin tüm erişimler de kaldırılacaktır. Devam etmek istiyor musunuz? | |||
| settings.deploy_key_deletion_success=Dağıtım anahtarı başarıyla silindi! | |||
| protected_branch=Koruma şube | |||
conf/locale/locale_zh-CN.ini
Outdated
| @@ -752,6 +752,17 @@ settings.add_key_success=新的部署密钥 '%s' 添加成功！ | |||
| settings.deploy_key_deletion=删除部署密钥 | |||
| settings.deploy_key_deletion_desc=删除该部署密钥会移除本仓库所以相关的操作权限。是否继续？ | |||
| settings.deploy_key_deletion_success=删除部署密钥成功！ | |||
| protected_branch=保护树枝 | |||
models/migrations/migrations.go
Outdated
| @@ -76,6 +76,8 @@ var migrations = []Migration{ | |||
|
|
|||
| // v13 -> v14:v0.9.87 | |||
| NewMigration("set comment updated with created", setCommentUpdatedWithCreated), | |||
| // v14 -> v15:v0.9.87 | |||
| NewMigration("set protect branches updated with created", setProtectedBranchUpdatedWithCreated), | |||
There was a problem hiding this comment.
New migrations should always go into the last row
cmd/update.go
Outdated
| @@ -48,6 +51,18 @@ func runUpdate(c *cli.Context) error { | |||
| log.GitLogger.Fatal(2, "First argument 'refName' is empty, shouldn't use") | |||
| } | |||
|
|
|||
| branchName := strings.TrimPrefix(args[0], git.BRANCH_PREFIX) | |||
| // UserID, _ := strconv.ParseInt(os.Getenv(models.ProtectedBranchUserID), 10, 64) | |||
cmd/update.go
Outdated
| @@ -48,6 +51,18 @@ func runUpdate(c *cli.Context) error { | |||
| log.GitLogger.Fatal(2, "First argument 'refName' is empty, shouldn't use") | |||
| } | |||
|
|
|||
| branchName := strings.TrimPrefix(args[0], git.BRANCH_PREFIX) | |||
| // UserID, _ := strconv.ParseInt(os.Getenv(models.ProtectedBranchUserID), 10, 64) | |||
| RepoID, _ := strconv.ParseInt(os.Getenv(models.ProtectedBranchRepoID), 10, 64) | |||
There was a problem hiding this comment.
I would say local variables start lowercase
modules/ssh/ssh.go
Outdated
| @@ -139,6 +139,7 @@ func listen(config *ssh.ServerConfig, port int) { | |||
| return | |||
| } | |||
|
|
|||
| os.Setenv("REMOTE_ADDR", sConn.RemoteAddr().String()) | |||
cmd/serve.go
Outdated
| @@ -273,6 +273,9 @@ func runServ(c *cli.Context) error { | |||
| } else { | |||
| gitcmd = exec.Command(verb, repoPath) | |||
| } | |||
| os.Setenv(models.ProtectedBranchUserID, fmt.Sprintf("%d", user.ID)) | |||
models/protected_branch.go
Outdated
| // Protected metadata | ||
| const ( | ||
| // Protected User ID | ||
| ProtectedBranchUserID = "__USER_ID" |
There was a problem hiding this comment.
Prefix them GITEA_, like this GITEA_USER_ID
There was a problem hiding this comment.
And I'd make it GITEA_PUSHER_ID since that makes more sense IMO
|
I'd need to test this before I LG_TM. I'll see if I have some time to spare this weekend |
|
Such a feature indeed requires manual testing |
|
@tboerger for now at least ;) |
models/protected_branch.go
Outdated
| } | ||
|
|
||
| if has, err := sess.Delete(ProtectedBranch); err != nil || has == 0 { | ||
| return err |
There was a problem hiding this comment.
There was a problem hiding this comment.
That's also a bug, I will send a PR to fix that.
There was a problem hiding this comment.
models/migrations/v15.go
Outdated
|
|
||
| if err = x.Sync2(new(ProtectedBranch)); err != nil { | ||
| return fmt.Errorf("Sync2: %v", err) | ||
| } else if _, err = x.Exec("UPDATE protected_branch SET updated_unix = created_unix"); err != nil { |
cmd/update.go
Outdated
| if accessMode == models.AccessModeWrite { | ||
| if protectBranch, err := models.GetProtectedBranchBy(repoID, branchName); err == nil { | ||
| if protectBranch != nil && !protectBranch.CanPush { | ||
| log.GitLogger.Fatal(2, "Protected Branch Cann't Push") |
There was a problem hiding this comment.
"... Can't ..." or "... Cannot ..."?
There was a problem hiding this comment.
I'd go for can not, since the others are abbreviations.
routers/repo/http.go
Outdated
| if protectBranch, err := models.GetProtectedBranchBy(repoID, branchName); err == nil { | ||
| log.Trace("%v", protectBranch) | ||
| if protectBranch != nil && !protectBranch.CanPush { | ||
| log.GitLogger.Error(2, "Protected Branch Cann't Push") |
There was a problem hiding this comment.
"... Can't ..." or "... Cannot ..."?
|
Please resolve the conflict and we could review and merge it. |
cmd/update.go
Outdated
| if accessMode == models.AccessModeWrite { | ||
| if protectBranch, err := models.GetProtectedBranchBy(repoID, branchName); err == nil { | ||
| if protectBranch != nil && !protectBranch.CanPush { | ||
| log.GitLogger.Fatal(2, "protected branch cannot be push") |
There was a problem hiding this comment.
protected branches can not be pushed to
models/migrations/migrations.go
Outdated
| NewMigration("create user column diff view style", createUserColumnDiffViewStyle), | ||
|
|
||
| // v14 -> v15:v0.9.87 | ||
| NewMigration("set protect branches updated with created", setProtectedBranchUpdatedWithCreated), |
There was a problem hiding this comment.
You don't need this migration, this is only required if you want to update content, the structure gets automatically migrated.
options/locale/locale_en-US.ini
Outdated
| 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't push |
options/locale/locale_en-US.ini
Outdated
| settings.add_protected_branch_success=Locked successfully | ||
| settings.remove_protected_branch_success=Unlocked successfully | ||
| settings.protected_branch_deletion=To delete a protected branch | ||
| settings.protected_branch_deletion_desc=The branch will be writable for developers. Are you sure? |
There was a problem hiding this comment.
maybe not use the term developers...
options/locale/locale_en-US.ini
Outdated
| settings.add_protected_branch_success=Locked successfully | ||
| settings.remove_protected_branch_success=Unlocked successfully | ||
| settings.protected_branch_deletion=To delete a protected branch | ||
| settings.protected_branch_deletion_desc=The branch will be writable. Are you sure? |
There was a problem hiding this comment.
Anyone with write permissions will be able to push directly to this branch. Are you sure?
|
LGTM |
|
It's ready now for review. |
| @@ -82,6 +82,8 @@ var migrations = []Migration{ | |||
| NewMigration("create user column allow create organization", createAllowCreateOrganizationColumn), | |||
| // V16 -> v17 | |||
| NewMigration("create repo unit table and add units for all repos", addUnitsToTables), | |||
| // v17 -> v18 | |||
| NewMigration("set protect branches updated with created", setProtectedBranchUpdatedWithCreated), | |||
There was a problem hiding this comment.
It's an entirely new struct, that doesn't need any migration because of the anyway executed sync function
There was a problem hiding this comment.
No. Sync on a migration is better safe. I have found the original method's bug, see #871. When you upgrade from an old version which haven't a column external_tracker_url, then a new version add this column by Sync not migration. but another new version will use this column, then the bug occupied.
|
For the record: Gogs got this via gogs/gogs@7e09d21 |
options/locale/TRANSLATORS
Outdated
| @@ -20,6 +20,7 @@ Cysioland | |||
| Damaris Padieu <damizx AT hotmail DOT fr> | |||
| Daniel Speichert <daniel AT speichert DOT pl> | |||
| David Yzaguirre <dvdyzag AT gmail DOT com> | |||
| Denis Denisov <denji0k AT gmail DOT com> | |||
There was a problem hiding this comment.
This should be added to all our repos ;)
options/locale/locale_ru-RU.ini
Outdated
| @@ -754,6 +754,18 @@ settings.add_key_success=Новый ключ развертывания '%s' у | |||
| settings.deploy_key_deletion=Удалить ключ развертывания | |||
| settings.deploy_key_deletion_desc=Удаление ключа развертывания приведет к удалению всех связанных прав доступа к репозиторию. Вы хотите продолжить? | |||
| settings.deploy_key_deletion_success=Ключ развертывания успешно удален! | |||
| settings.branches=Ветки | |||
There was a problem hiding this comment.
Since we switched to crowdin this have to be done within crowdin, otherwise we get into issues while we try to sync it.
| @@ -0,0 +1,99 @@ | |||
| {{template "base/head" .}} | |||
There was a problem hiding this comment.
This file is generally not really good indented. Please fix the indentation.
templates/repo/settings/navbar.tmpl
Outdated
| @@ -7,6 +7,11 @@ | |||
| <a class="{{if .PageIsSettingsCollaboration}}active{{end}} item" href="{{.RepoLink}}/settings/collaboration"> | |||
| {{.i18n.Tr "repo.settings.collaboration"}} | |||
| </a> | |||
| {{if not .Repository.IsBare}} | |||
| <a class="{{if .PageIsSettingsBranches}}active{{end}} item" href="{{.RepoLink}}/settings/branches"> | |||
cmd/update.go
Outdated
|
|
||
| if protectBranch != nil { | ||
| log.GitLogger.Fatal(2, "protected branches can not be pushed to") | ||
| fail("protected branches can not be pushed to", "protected branches can not be pushed to") |
There was a problem hiding this comment.
is .Fatal not really fatal that you add a fail() call afterwards ?
|
I've given this code a try but I'm refused any push, even to branches which are not protected. The server (http.log) says: |
|
Also, the "Delete" button doesn't work, despite endpoint returnin 200 OK the protected branches are still listed in the "Branches" page in repository setting. |
|
Yes. I'm fixing that. |
|
@strk please confirm. |
|
@lunny just tried but still doesn't work for me. I hit "Delete" on a protected branch and it still sits there. |
|
@strk please delete the old branch and fetch the newest branch. |
|
LGTM after rebasing to current master (70ae6d1) and testing briefly. Note that the Gogs version of protected branches has a more fine-grained configuration for branch protection, you can check it on try.gogs.io. |
tboerger
added
lgtm/done
|
@strk Yes. We will improve the protected branch on v1.2. |
gogs/gogs@7e09d21, #32 (
gogs/gogs#3921):org/:reponame/settings/branches).