Protected branches system#339

Merged

lunny merged 6 commits into

go-gitea:masterfrom

denji:protect_branch

Feb 21, 2017

Contributor

denji commented Dec 2, 2016 •

edited

Loading

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

Member

tboerger commented Dec 2, 2016

Tracking issue is #32

tboerger added lgtm/need 2 type/feature and removed lgtm/need 2 labels

bkcsoft suggested changes

View reviewed changes

Contributor

bkcsoft left a comment

2 things for Gitea acceptance :)

cmd/update.go Outdated

Contributor

bkcsoft Dec 2, 2016

Wrong module :)

models/protected_branch.go Outdated

Contributor

bkcsoft Dec 2, 2016

These should be in CamelCase :)

strk reviewed

View reviewed changes

models/protected_branch.go Outdated

Member

strk Dec 2, 2016

It's 2016 (almost 2017!) :)

Contributor

bkcsoft Dec 2, 2016

Also Gitea Authors ;)

Contributor Author

denji Dec 2, 2016 •

edited

Loading

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

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 added the lgtm/need 2 label

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 changed the title ~~Protected branch (gogits/gogs#3921)~~ Protected branch

tboerger added this to the 1.0.0 milestone

denji changed the title ~~Protected branch~~ Protected branches system

tboerger suggested changes

View reviewed changes

Member

tboerger left a comment

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

conf/locale/locale_de-DE.ini Outdated

Member

tboerger Dec 2, 2016

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

conf/locale/locale_en-US.ini Outdated

Member

tboerger Dec 2, 2016

Branch protection

conf/locale/locale_en-US.ini Outdated

Member

tboerger Dec 2, 2016

Enable protection

conf/locale/locale_en-US.ini Outdated

Member

tboerger Dec 2, 2016

Disable protection

conf/locale/locale_fi-FI.ini Outdated

Member

tboerger Dec 2, 2016

Is this also Google translate?

conf/locale/locale_sr-SP.ini Outdated

Member

tboerger Dec 2, 2016

Is this also Google translate?

conf/locale/locale_sv-SE.ini Outdated

Member

tboerger Dec 2, 2016

Is this also Google translate?

conf/locale/locale_tr-TR.ini Outdated

Member

tboerger Dec 2, 2016

Is this also Google translate?

conf/locale/locale_zh-CN.ini Outdated

Member

tboerger Dec 2, 2016

Is this also Google translate?

Member

lunny Dec 3, 2016

I think yes

models/migrations/migrations.go Outdated

Member

tboerger Dec 2, 2016

New migrations should always go into the last row

tboerger reviewed

View reviewed changes

cmd/update.go Outdated

Member

tboerger Dec 2, 2016

Don't need that? Than drop it?

cmd/update.go Outdated

Member

tboerger Dec 2, 2016

I would say local variables start lowercase

bkcsoft suggested changes

View reviewed changes

modules/ssh/ssh.go Outdated

Contributor

bkcsoft Dec 2, 2016

Why is this added?

cmd/serve.go Outdated

Contributor

bkcsoft Dec 2, 2016

This is never used...

models/protected_branch.go Outdated

Contributor

bkcsoft Dec 2, 2016 •

edited

Loading

Prefix them GITEA_, like this GITEA_USER_ID

Contributor

bkcsoft Dec 2, 2016

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

bkcsoft approved these changes

View reviewed changes

Contributor

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

Member

tboerger commented Dec 2, 2016

Such a feature indeed requires manual testing

Contributor

bkcsoft commented Dec 2, 2016

@tboerger for now at least ;)

bkcsoft modified the milestones: 1.1.0, 1.0.0

lunny requested changes

View reviewed changes

models/protected_branch.go Outdated

Member

lunny Dec 14, 2016

When has == 0 then err == nil

Contributor Author

denji Dec 14, 2016 •

edited

Loading

@lunny models/repo_collaboration.go#L160

Member

lunny Dec 14, 2016

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

Member

lunny Dec 31, 2016

This line is OK.

Contributor Author

denji Dec 31, 2016

@lunny go-xorm/xorm@d75356f ?

models/migrations/v15.go Outdated

Member

lunny Dec 14, 2016

this code is no meaning

ethantkoenig reviewed

View reviewed changes

cmd/update.go Outdated

Member

ethantkoenig Dec 14, 2016

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

Contributor

bkcsoft Dec 20, 2016

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

routers/repo/http.go Outdated

Member

ethantkoenig Dec 14, 2016

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

Member

lunny commented Dec 24, 2016

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

bkcsoft suggested changes

View reviewed changes

Contributor

bkcsoft left a comment

Slight grammar fix 🙂

cmd/update.go Outdated

Contributor

bkcsoft Dec 25, 2016

protected branches can not be pushed to

tboerger reviewed

View reviewed changes

models/migrations/migrations.go Outdated

Member

tboerger Dec 29, 2016

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

bkcsoft suggested changes

View reviewed changes

options/locale/locale_en-US.ini Outdated

Contributor

bkcsoft Dec 29, 2016

You can not push

options/locale/locale_en-US.ini Outdated

Contributor

bkcsoft Dec 29, 2016

maybe not use the term developers...

bkcsoft suggested changes

View reviewed changes

Contributor

bkcsoft left a comment

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

options/locale/locale_en-US.ini Outdated

Contributor

bkcsoft Dec 29, 2016

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

bkcsoft approved these changes

View reviewed changes

Contributor

bkcsoft commented Dec 30, 2016

LGTM

denji and others added 2 commits

February 9, 2017 07:51


          reformat tmpl

4fc3563


          remove unused comment

92e5ae3

Member

lunny commented Feb 9, 2017

It's ready now for review.

lunny removed the pr/wip label

bkcsoft approved these changes

View reviewed changes

tboerger reviewed

View reviewed changes

models/migrations/migrations.go

               	// V16 -> v17
               	NewMigration("create repo unit table and add units for all repos", addUnitsToTables),
+              	// v17 -> v18
+              	NewMigration("set protect branches updated with created", setProtectedBranchUpdatedWithCreated),

Member

tboerger Feb 11, 2017

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

Member

lunny Feb 11, 2017

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.

ethantkoenig approved these changes

View reviewed changes

Member

strk commented Feb 20, 2017

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

tboerger reviewed

View reviewed changes

options/locale/TRANSLATORS Outdated

               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>

Member

tboerger Feb 20, 2017

This should be added to all our repos ;)

Member

lunny Feb 20, 2017

removed this.

tboerger reviewed

View reviewed changes

options/locale/locale_ru-RU.ini Outdated

               settings.deploy_key_deletion=Удалить ключ развертывания
               settings.deploy_key_deletion_desc=Удаление ключа развертывания приведет к удалению всех связанных прав доступа к репозиторию. Вы хотите продолжить?
               settings.deploy_key_deletion_success=Ключ развертывания успешно удален!
+              settings.branches=Ветки

Member

tboerger Feb 20, 2017

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

Member

lunny Feb 20, 2017

removed this.

tboerger reviewed

View reviewed changes

templates/repo/settings/branches.tmpl

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

Member

tboerger Feb 20, 2017

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

Member

lunny Feb 20, 2017

done.

tboerger reviewed

View reviewed changes

templates/repo/settings/navbar.tmpl Outdated

               			{{.i18n.Tr "repo.settings.collaboration"}}
               		</a>
+              		{{if not .Repository.IsBare}}
+              		<a class="{{if .PageIsSettingsBranches}}active{{end}} item" href="{{.RepoLink}}/settings/branches">

Member

tboerger Feb 20, 2017

Indent by one level

tboerger approved these changes

View reviewed changes

Member

tboerger left a comment

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

strk reviewed

View reviewed changes

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")

Member

strk Feb 20, 2017

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

Member

lunny Feb 20, 2017

indeed a problem.

Member

lunny Feb 20, 2017

done.

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


          indent all the template files and remove ru translations since we use…

ab01219

… crowdin

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.

Member

lunny commented Feb 20, 2017

Yes. I'm fixing that.


          fix the push bug

Member

lunny commented Feb 20, 2017

@strk please confirm.

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.

Member

lunny commented Feb 21, 2017

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

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 added lgtm/done and removed lgtm/need 1 labels

Member

lunny commented Feb 21, 2017

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

lunny merged commit fd941db into go-gitea:master

This was referenced Feb 22, 2017

Protected branches system #32

Closed

Gitea hosted Gitea #1029

Open

go-gitea locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

lunny lunny approved these changes

+5 more reviewers

strk strk left review comments

lcges lcges left review comments

tboerger tboerger approved these changes

bkcsoft bkcsoft approved these changes

ethantkoenig ethantkoenig approved these changes

Labels

lgtm/done type/feature