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

Fix adding branch as protected to not allow pushing to it #2556

Merged
merged 4 commits into from
Sep 20, 2017

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Sep 20, 2017

Fixes #2517

This PR targets 1.2 branch as bug is present only in it. In master branch it works fine as @lunny did rewrite protected branch UI & functionality with whitelist support

@lafriks lafriks added issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP labels Sep 20, 2017
@lafriks lafriks added this to the 1.2.0 milestone Sep 20, 2017
@lunny
Copy link
Member

lunny commented Sep 20, 2017

LGTM

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Sep 20, 2017
@daviian
Copy link
Member

daviian commented Sep 20, 2017

@lafriks IMO there should be a migration from old protected branches as well, since existing protected branches do not work after that change.

@lafriks
Copy link
Member Author

lafriks commented Sep 20, 2017

@daviian they will not work only for those who added protected branches in 1.2-rcX. Protected branches when upgrading from 1.1 to 1.2 they will work and adding new migration here would break upgrade path from 1.2 to 1.3 later

@daviian
Copy link
Member

daviian commented Sep 20, 2017

@lafriks But when upgrading from 1.1 to 1.2 existing protected branches have can_push=true, so after this PR they are not protected and need to be readded.

@daviian
Copy link
Member

daviian commented Sep 20, 2017

I would suggest a quick'n'dirty workaround to release v1.2 since it is already fixed in current master.

Simply remove the CanPush check in https://github.com/lafriks/gitea/blob/1773e88643a2df7e7efbe86ec424409b45a4d576/cmd/hook.go#L130

@lunny
Copy link
Member

lunny commented Sep 20, 2017

@daviian in fact, canpush never be used, when remove from protected branch, the database record will be deleted not updated canpush to false.

@lunny
Copy link
Member

lunny commented Sep 20, 2017

that means, this PR will not break anything, I think.

@lafriks
Copy link
Member Author

lafriks commented Sep 20, 2017

@lunny @daviian no private.GetProtectedBranchBy(repoID, branchName) will not return nil when there is no protected record, it will just return type object with CanPush: true set. I might add new migration to both master & 1.2 than that updates all rows to can_push = false.

@lafriks
Copy link
Member Author

lafriks commented Sep 20, 2017

also this column will be needed anyway later to be able to set that branch can be pushed to always but are protected only from deletion

@daviian
Copy link
Member

daviian commented Sep 20, 2017

@lunny @lafriks I've simulated an update from 1.1.4 to 1.2 (including this PR) with a fresh database (only one new created repository and a protected branch in it) and everything and the PR in its current state doesn't fix the issue.
So I agree with @lafriks to create a migration in master and 1.2

@lafriks
Copy link
Member Author

lafriks commented Sep 20, 2017

@lunny @daviian Migration PR that I will chery-pick to this PR when it is merged to master: #2560

@lafriks
Copy link
Member Author

lafriks commented Sep 20, 2017

@daviian @lunny migration added so upgrade path should work just fine now also

@daviian
Copy link
Member

daviian commented Sep 20, 2017

@lafriks Tested and works like a charm 👍

@daviian
Copy link
Member

daviian commented Sep 20, 2017

LGTM

@daviian
Copy link
Member

daviian commented Sep 20, 2017

Make LG-TM work

@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 Sep 20, 2017
@lafriks
Copy link
Member Author

lafriks commented Sep 20, 2017

Make LG-TM work again

@lafriks lafriks merged commit 25e71ad into go-gitea:release/v1.2 Sep 20, 2017
@lafriks lafriks deleted the fix_protected_branches branch September 20, 2017 17:14
@lafriks lafriks mentioned this pull request Sep 20, 2017
7 tasks
@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
issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants