-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 deletion of unprotected branches #2630
Fix deletion of unprotected branches #2630
Conversation
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for unprotected protectBranch
should be nil & CanPush
should be used only for pushing code check not for branch deletion
@lafriks I can change internal api too. At the moment the internal api returns a protectedBranch with canPush: true if there is none. |
I think that would be correct approach as this is also what master branch does |
@lafriks done |
@daviian please fix go fmt error |
Signed-off-by: David Schneiderbauer <[email protected]>
LGTM |
@@ -38,6 +38,11 @@ func (protectBranch *ProtectedBranch) BeforeUpdate() { | |||
protectBranch.UpdatedUnix = time.Now().Unix() | |||
} | |||
|
|||
// IsProtected returns if the branch is protected | |||
func (protectBranch *ProtectedBranch) IsProtected() bool { | |||
return protectBranch.ID > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daviian you can change here to protectBranch != nil && protectBranch.ID > 0
and than you can remove that nil check above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lafriks Current implementation would reflect current master. But if you want, of course I can add the check inside the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daviian ok, let it be this way than for now, this could be optimized in master than
LGTM |
Targets #2629
PR only targets release v1.2 as it is already fixed on master