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

add API endpoints to update/delete protected branch #7093

Closed
wants to merge 3 commits into from

Conversation

quantonganh
Copy link
Contributor

Fix #2922

@techknowlogick
Copy link
Member

Thanks for PR :) The CI is currently failing due to needing a rebase on the latest commit of master.

I think additional routes for the API are needing for these methods, as well as swagger definitions.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 31, 2019
@codecov-io
Copy link

codecov-io commented Jun 3, 2019

Codecov Report

Merging #7093 into master will decrease coverage by 0.67%.
The diff coverage is 6.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7093      +/-   ##
==========================================
- Coverage    41.8%   41.12%   -0.68%     
==========================================
  Files         496      469      -27     
  Lines       65586    63604    -1982     
==========================================
- Hits        27416    26159    -1257     
+ Misses      34654    34027     -627     
+ Partials     3516     3418      -98
Impacted Files Coverage Δ
routers/api/v1/api.go 71.53% <100%> (-1.47%) ⬇️
routers/api/v1/convert/convert.go 84.03% <100%> (+2.62%) ⬆️
routers/api/v1/repo/branch.go 14.74% <2.23%> (-29.01%) ⬇️
modules/upload/filetype.go 0% <0%> (-72.23%) ⬇️
routers/api/v1/repo/status.go 21.02% <0%> (-23.52%) ⬇️
models/repo_unit.go 64.91% <0%> (-21.06%) ⬇️
modules/setting/git.go 46.15% <0%> (-19.24%) ⬇️
modules/git/notes.go 40% <0%> (-17.15%) ⬇️
routers/api/v1/org/team.go 25.58% <0%> (-12.61%) ⬇️
models/topic.go 54.7% <0%> (-11.97%) ⬇️
... and 218 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 f92a0b6...4afd393. Read the comment docs.

@lunny lunny added the pr/wip This PR is not ready for review label Jun 3, 2019
@quantonganh quantonganh force-pushed the fix-2922 branch 3 times, most recently from 85243c9 to 9c4976a Compare June 11, 2019 05:43
@quantonganh
Copy link
Contributor Author

I think additional routes for the API are needing for these methods, as well as swagger definitions.

Routes and swagger definitions have been added.
@techknowlogick Could you please take a review?

@techknowlogick techknowlogick added type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed pr/wip This PR is not ready for review labels Jun 11, 2019
@techknowlogick techknowlogick added this to the 1.10.0 milestone Jun 11, 2019
protectBranch, err := models.GetProtectedBranchBy(ctx.Repo.Repository.ID, ctx.Params(":branch"))
if err != nil {
if !git.IsErrBranchNotExist(err) {
ctx.Error(http.StatusInternalServerError, "GetProtectedBranchBy", err)
return
}
Copy link
Contributor

@davidsvantesson davidsvantesson Aug 27, 2019

Choose a reason for hiding this comment

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

Xorm will not give an error if there is no row in the database, but an empty object.
You need to handle:

  • If there is no protected branch, but the branch exist (create protectBranch with default values / protection:false )
  • If the branch doesn't exist. (404)

if !git.IsErrBranchNotExist(err) {
ctx.Error(http.StatusInternalServerError, "GetProtectedBranchBy", err)
return
}
Copy link
Contributor

@davidsvantesson davidsvantesson Aug 27, 2019

Choose a reason for hiding this comment

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

Same problem as above with xorm.
If the protected branch does not exist, you need to check if the branch exist (or give 404)

if !git.IsErrBranchNotExist(err) {
ctx.Error(http.StatusInternalServerError, "GetProtectedBranchBy", err)
return
}
Copy link
Contributor

@davidsvantesson davidsvantesson Aug 27, 2019

Choose a reason for hiding this comment

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

Same problem as above with xorm. Maybe ok to give 404 directly if protected branch doesn't exist.

@@ -9115,6 +9243,44 @@
},
"x-go-package": "code.gitea.io/gitea/modules/structs"
},
"ProtectBranchForm": {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit problematic to reuse the same structure for API from documentation point of view. How do I know the format of XWhiteListTeams and XWhiteListUsers? Preferably they should be arrays. At least they should change name to XWhiteListTeamIDs and XWhiteListUserIDs

Commit *PayloadCommit `json:"commit"`
Name string `json:"name"`
Commit *PayloadCommit `json:"commit"`
Protected bool `json:"protected"`
}
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 we need more details about the branch protection settings but not only a bool.

@davidsvantesson
Copy link
Contributor

I think this one can be moved to 1.12, no chance for this release. I hope either this one or my new PR can get into next release.

@techknowlogick techknowlogick modified the milestones: 1.11.0, 1.12.0 Dec 12, 2019
@stale
Copy link

stale bot commented Feb 10, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Feb 10, 2020
@davidsvantesson
Copy link
Contributor

This one can be closed.

@stale stale bot removed the issue/stale label Feb 13, 2020
@lunny lunny closed this Feb 13, 2020
@lunny lunny removed this from the 1.12.0 milestone Feb 13, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API endpoint to configure default branch and protected branches is missing
6 participants