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.

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

Protected branches system #339

Merged
merged 6 commits into from
Feb 21, 2017
Merged

Protected branches system #339

merged 6 commits into from
Feb 21, 2017

Conversation

denji
Copy link
Contributor

@denji denji commented Dec 2, 2016

gogs/gogs@7e09d21, #32 (gogs/gogs#3921)

  • Moved default branch to branches section (:org/:reponame/settings/branches).
  • Initial support Protected Branch.
    • Admin does not restrict
    • Owner not to limit
    • To write permission restrictions

@tboerger
Copy link
Member

tboerger commented Dec 2, 2016

Tracking issue is #32

@tboerger tboerger added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 2, 2016
Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

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

2 things for Gitea acceptance :)

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"
Copy link
Member

Choose a reason for hiding this comment

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

Wrong module :)

)

const (
PROTECTED_BRANCH_USER_ID = "__UER_ID"
Copy link
Member

Choose a reason for hiding this comment

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

These should be in CamelCase :)

@@ -0,0 +1,138 @@
// Copyright 2014 The Gogs Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

It's 2016 (almost 2017!) :)

Copy link
Member

Choose a reason for hiding this comment

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

Also Gitea Authors ;)

Copy link
Contributor Author

@denji denji Dec 2, 2016

Choose a reason for hiding this comment

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

+// Copyright 2016 The Gitea Authors. All rights reserved.
// Copyright 2014 The Gogs Authors. All rights reserved.

@strk
Copy link
Member

strk commented Dec 2, 2016

Please also take your time to document all new methods and functions.
And ideally also see if you can add automated testcases for the new functions.
This will have to wait for 1.1.0 anyway (please someone add that milestone)

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 2, 2016
@strk
Copy link
Member

strk commented Dec 2, 2016

@denji please on next squash-rebase change the issue referenc from gogs/gogs#3921 to #32 (gitea issue)

@denji denji changed the title Protected branch (gogits/gogs#3921) Protected branch Dec 2, 2016
@tboerger tboerger added this to the 1.0.0 milestone Dec 2, 2016
@denji denji changed the title Protected branch Protected branches system Dec 2, 2016
Copy link
Member

@tboerger tboerger left a comment

Choose a reason for hiding this comment

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

Please translate only languages you are familiar with, don't use something like Google translate.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use Google translate, these translations doesn't make sense at all

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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
settings.add_protected_branch=Unprotection branch
Copy link
Member

Choose a reason for hiding this comment

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

Enable protection

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
Copy link
Member

Choose a reason for hiding this comment

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

Disable protection

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Is this also Google translate?

@@ -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=Заштита грана
Copy link
Member

Choose a reason for hiding this comment

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

Is this also Google translate?

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Is this also Google translate?

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Is this also Google translate?

@@ -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=保护树枝
Copy link
Member

Choose a reason for hiding this comment

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

Is this also Google translate?

Copy link
Member

Choose a reason for hiding this comment

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

I think yes

@@ -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),
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

Don't need that? Than drop it?

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)
Copy link
Member

Choose a reason for hiding this comment

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

I would say local variables start lowercase

@@ -139,6 +139,7 @@ func listen(config *ssh.ServerConfig, port int) {
return
}

os.Setenv("REMOTE_ADDR", sConn.RemoteAddr().String())
Copy link
Member

Choose a reason for hiding this comment

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

Why is this added?

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))
Copy link
Member

Choose a reason for hiding this comment

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

This is never used...

// Protected metadata
const (
// Protected User ID
ProtectedBranchUserID = "__USER_ID"
Copy link
Member

@bkcsoft bkcsoft Dec 2, 2016

Choose a reason for hiding this comment

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

Prefix them GITEA_, like this GITEA_USER_ID

Copy link
Member

Choose a reason for hiding this comment

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

And I'd make it GITEA_PUSHER_ID since that makes more sense IMO

@bkcsoft
Copy link
Member

bkcsoft commented Dec 2, 2016

I'd need to test this before I LG_TM. I'll see if I have some time to spare this weekend

@tboerger
Copy link
Member

tboerger commented Dec 2, 2016

Such a feature indeed requires manual testing

@bkcsoft
Copy link
Member

bkcsoft commented Dec 2, 2016

@tboerger for now at least ;)

@bkcsoft bkcsoft modified the milestones: 1.1.0, 1.0.0 Dec 3, 2016
}

if has, err := sess.Delete(ProtectedBranch); err != nil || has == 0 {
return err
Copy link
Member

Choose a reason for hiding this comment

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

When has == 0 then err == nil

Copy link
Contributor Author

@denji denji Dec 14, 2016

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That's also a bug, I will send a PR to fix that.

Copy link
Member

Choose a reason for hiding this comment

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

This line is OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


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 {
Copy link
Member

Choose a reason for hiding this comment

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

this code is no meaning

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")
Copy link
Member

Choose a reason for hiding this comment

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

"... Can't ..." or "... Cannot ..."?

Copy link
Member

Choose a reason for hiding this comment

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

I'd go for can not, since the others are abbreviations.

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")
Copy link
Member

Choose a reason for hiding this comment

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

"... Can't ..." or "... Cannot ..."?

@lunny
Copy link
Member

lunny commented Dec 24, 2016

Please resolve the conflict and we could review and merge it.

Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

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

Slight grammar fix 🙂

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")
Copy link
Member

Choose a reason for hiding this comment

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

protected branches can not be pushed to

NewMigration("create user column diff view style", createUserColumnDiffViewStyle),

// v14 -> v15:v0.9.87
NewMigration("set protect branches updated with created", setProtectedBranchUpdatedWithCreated),
Copy link
Member

Choose a reason for hiding this comment

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

You don't need this migration, this is only required if you want to update content, the structure gets automatically migrated.

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
Copy link
Member

Choose a reason for hiding this comment

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

You can not push

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?
Copy link
Member

Choose a reason for hiding this comment

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

maybe not use the term developers...

Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

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

Sorry that I'm being a pain... :(

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?
Copy link
Member

Choose a reason for hiding this comment

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

Anyone with write permissions will be able to push directly to this branch. Are you sure?

@bkcsoft
Copy link
Member

bkcsoft commented Dec 30, 2016

LGTM

@lunny
Copy link
Member

lunny commented Feb 9, 2017

It's ready now for review.

@lunny lunny removed the pr/wip This PR is not ready for review label Feb 9, 2017
@@ -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),
Copy link
Member

Choose a reason for hiding this comment

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

It's an entirely new struct, that doesn't need any migration because of the anyway executed sync function

Copy link
Member

Choose a reason for hiding this comment

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

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.

@strk
Copy link
Member

strk commented Feb 20, 2017

For the record: Gogs got this via gogs/gogs@7e09d21

@@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

This should be added to all our repos ;)

Copy link
Member

Choose a reason for hiding this comment

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

removed this.

@@ -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=Ветки
Copy link
Member

Choose a reason for hiding this comment

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

Since we switched to crowdin this have to be done within crowdin, otherwise we get into issues while we try to sync it.

Copy link
Member

Choose a reason for hiding this comment

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

removed this.

@@ -0,0 +1,99 @@
{{template "base/head" .}}
Copy link
Member

Choose a reason for hiding this comment

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

This file is generally not really good indented. Please fix the indentation.

Copy link
Member

Choose a reason for hiding this comment

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

done.

@@ -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">
Copy link
Member

Choose a reason for hiding this comment

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

Indent by one level

Copy link
Member

@tboerger tboerger left a comment

Choose a reason for hiding this comment

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

I have added some minor comments, but otherwise I'm fine with that.

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")
Copy link
Member

Choose a reason for hiding this comment

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

is .Fatal not really fatal that you add a fail() call afterwards ?

Copy link
Member

Choose a reason for hiding this comment

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

indeed a problem.

Copy link
Member

Choose a reason for hiding this comment

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

done.

@strk
Copy link
Member

strk commented Feb 20, 2017

I've given this code a try but I'm refused any push, even to branches which are not protected.
The client says:

error: RPC failed; result=22, HTTP code = 403
fatal: The remote end hung up unexpectedly
fatal: The remote end hung up unexpectedly
Everything up-to-date

The server (http.log) says:

2017/02/20 14:58:17 [...routers/repo/http.go:462 serviceRPC()] [E] protected branches can not be pushed to

@strk
Copy link
Member

strk commented Feb 20, 2017

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.

@lunny
Copy link
Member

lunny commented Feb 20, 2017

Yes. I'm fixing that.

@lunny
Copy link
Member

lunny commented Feb 20, 2017

@strk please confirm.

@strk
Copy link
Member

strk commented Feb 20, 2017

@lunny just tried but still doesn't work for me. I hit "Delete" on a protected branch and it still sits there.
Also, I cannot push to a new branch either.

@lunny
Copy link
Member

lunny commented Feb 21, 2017

@strk please delete the old branch and fetch the newest branch.

@strk
Copy link
Member

strk commented Feb 21, 2017

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 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 Feb 21, 2017
@lunny
Copy link
Member

lunny commented Feb 21, 2017

@strk Yes. We will improve the protected branch on v1.2.

@lunny lunny merged commit fd941db into go-gitea:master Feb 21, 2017
This was referenced Feb 22, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants