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

Approvals at Branch Protection #5350

Merged
merged 14 commits into from Dec 11, 2018

Conversation

6 participants
@jonasfranz
Copy link
Member

jonasfranz commented Nov 18, 2018

Fixes #5251

This PR adds approvals limitations to branch protection. A pitfall is that it is currently only possible to assign Teams/Users with write permissions to branch protection. I've removed this for teams but not for users.

Screenshots

screenshot_2018-11-18 approvals 3 screenshot_2018-11-18 approvals 2
screenshot_2018-11-18 approvals 1 screenshot_2018-11-18 approvals

jonasfranz added some commits Nov 10, 2018

Add branch protection for approvals
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Add required approvals
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Add missing comments and fmt
Signed-off-by: Jonas Franz <info@jonasfranz.software>

@jonasfranz jonasfranz added this to the 1.7.0 milestone Nov 18, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 18, 2018

Codecov Report

Merging #5350 into master will decrease coverage by <.01%.
The diff coverage is 25.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5350      +/-   ##
==========================================
- Coverage   37.61%   37.61%   -0.01%     
==========================================
  Files         317      318       +1     
  Lines       46835    46928      +93     
==========================================
+ Hits        17619    17650      +31     
- Misses      26712    26770      +58     
- Partials     2504     2508       +4
Impacted Files Coverage Δ
models/migrations/migrations.go 2.61% <ø> (ø) ⬆️
modules/auth/repo_form.go 39% <ø> (ø) ⬆️
routers/repo/issue.go 37.28% <0%> (-0.28%) ⬇️
models/review.go 59.77% <0%> (-4.3%) ⬇️
models/migrations/v74.go 0% <0%> (ø)
models/branches.go 43.33% <25.53%> (-4%) ⬇️
models/org_team.go 49.3% <40%> (-0.1%) ⬇️
routers/repo/setting_protected_branch.go 40.23% <43.47%> (-0.04%) ⬇️
models/pull.go 51.42% <75%> (+0.15%) ⬆️
... and 2 more

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 64680b7...cf95a76. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 label Nov 18, 2018

Show resolved Hide resolved models/branches.go Outdated
@lunny

This comment has been minimized.

Copy link
Member

lunny commented Nov 18, 2018

@JonasFranzDEV any screenshot?

@jonasfranz

This comment has been minimized.

Copy link
Member Author

jonasfranz commented Nov 18, 2018

@lunny fixed your first comment and added screenshots.

jonasfranz added some commits Nov 18, 2018

Show resolved Hide resolved models/review.go

@bkcsoft bkcsoft added lgtm/need 1 and removed lgtm/need 2 labels Nov 21, 2018

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Nov 21, 2018

I think this depends on #5132 to have single method that returns latest per-user reviews

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Nov 22, 2018

right, blocked by #5132

@lafriks lafriks added the changelog label Nov 22, 2018

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Nov 22, 2018

Do we allow merging if there is request for changes?

@kolaente

This comment has been minimized.

Copy link
Member

kolaente commented Nov 22, 2018

@lafriks I'd make this an option which could be toggled on or off in the settings (default not allowing)

@jonasfranz

This comment has been minimized.

Copy link
Member Author

jonasfranz commented Nov 22, 2018

@lafriks I think that is out of scope of this PR since this just checks if there are at least X approvals by granted users. But it will only check for the latest approval/rejection of a user as @lunny mentioned.

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Nov 22, 2018

Yes this can be added in other PR as an option

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Nov 22, 2018

@lafriks I think @JonasFranzDEV is right, allow merging or not when there is request for changes should be another PR.

jonasfranz added some commits Nov 24, 2018

Merge branch 'feature/branch-protection-approvals' of github.com:Jona…
…sFranzDEV/gitea into feature/branch-protection-approvals

Signed-off-by: Jonas Franz <info@jonasfranz.software>

# Conflicts:
#	models/review_test.go
Add migration for approval whitelists
Signed-off-by: Jonas Franz <info@jonasfranz.software>

@lunny lunny referenced this pull request Nov 26, 2018

Closed

Pull request approvals #2417

@jonasfranz

This comment has been minimized.

Copy link
Member Author

jonasfranz commented Nov 28, 2018

@lunny need your review

@bkcsoft bkcsoft added lgtm/done and removed lgtm/need 1 labels Nov 28, 2018

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Nov 29, 2018

file conflict.

jonasfranz and others added some commits Nov 30, 2018

@jonasfranz

This comment has been minimized.

Copy link
Member Author

jonasfranz commented Nov 30, 2018

@lunny File conflicts resolved

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Dec 1, 2018

Pull detail page will result in 500 on MSSQL.
image

@kolaente

This comment has been minimized.

Copy link
Member

kolaente commented Dec 2, 2018

@lunny is this using the function from Review summaries?

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Dec 3, 2018

@kolaente yes, I think that's the reason.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Dec 11, 2018

Maybe this could be resolved by #5502

@jonasfranz

This comment has been minimized.

Copy link
Member Author

jonasfranz commented Dec 11, 2018

Can you verify that? @lunny

@lunny

lunny approved these changes Dec 11, 2018

@lunny lunny merged commit 9681c83 into go-gitea:master Dec 11, 2018

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
@lunny

This comment has been minimized.

Copy link
Member

lunny commented Dec 11, 2018

@JonasFranzDEV it seems it's has some bug on Mssql. See
image

@lunny lunny referenced this pull request Dec 11, 2018

Merged

fix approvals limitation #5521

@jonasfranz jonasfranz deleted the jonasfranz:feature/branch-protection-approvals branch Dec 13, 2018

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