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 Endpoint for Branch Creation #11607

Merged
merged 8 commits into from
May 29, 2020

Conversation

tle-huu
Copy link
Contributor

@tle-huu tle-huu commented May 24, 2020

Issue: #11376

This commit introduces an API endpoint for branch creation.
The added route is POST /repos/{owner}/{repo}/branches.

A JSON with the name of the new branch and the name of the "old" branch is required as parameters.

@techknowlogick techknowlogick added this to the 1.13.0 milestone May 24, 2020
@techknowlogick techknowlogick added modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels May 24, 2020
@techknowlogick
Copy link
Member

Thanks for PR :) It looks like you have modified the vendors/modules.txt file, this is likely due to working with an older version of Go. Could you revert your changes from that specific file?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 24, 2020
@tle-huu tle-huu force-pushed the terence/add_create_branch_to_api branch from e34cddc to 4aafa8b Compare May 24, 2020 17:03
@tle-huu
Copy link
Contributor Author

tle-huu commented May 24, 2020

Thanks for PR :) It looks like you have modified the vendors/modules.txt file, this is likely due to working with an older version of Go. Could you revert your changes from that specific file?

Oh sorry, I amended

modules/structs/repo.go Outdated Show resolved Hide resolved
modules/structs/repo.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 25, 2020
@zeripath zeripath changed the title [FEATURE] [API] Add Endpoint for Branch Creation Add API Endpoint for Branch Creation May 25, 2020
routers/api/v1/api.go Outdated Show resolved Hide resolved
modules/structs/repo.go Show resolved Hide resolved
routers/api/v1/repo/branch.go Show resolved Hide resolved
@tle-huu tle-huu force-pushed the terence/add_create_branch_to_api branch 2 times, most recently from 997c392 to f2978d9 Compare May 28, 2020 13:24
@6543
Copy link
Member

6543 commented May 28, 2020

since you dont use RepoRefByType you have to:

  • check if ctx.Repo.Repository.IsEmpty { first

and since repo_module.CreateNewBranch return no specific error if the OldBranch not exist,
at the moment it will cause a 500 errror
Either:

  • create a custom error and extend the function CreateNewBranch here:
    if !git.IsBranchExist(repo.RepoPath(), oldBranchName) {
    return fmt.Errorf("OldBranch: %s does not exist. Cannot create new branch from this", oldBranchName)
    }
  • a 404 responce is expected (has to be added to the swagger docs too)

Or:

  • check via !ctx.Repo.GitRepo.IsBranchExist(opt.OldBranchName) bevore this function is called
  • a 404 responce is expected (has to be added to the swagger docs too)

After this Only some Tests are missing :)

@6543
Copy link
Member

6543 commented May 28, 2020

and a format question why do you created a second function (CreateRepoBranch) if you can just put the code into CreateBranch.

It would make sence if you like to re-use CreateRepoBranch but it is only used once

@6543
Copy link
Member

6543 commented May 28, 2020

@tle-huu thanks for the pull, after the points above are resolved I think its good to go ;)

@tle-huu tle-huu force-pushed the terence/add_create_branch_to_api branch from 0d1bd15 to 2ce8212 Compare May 28, 2020 14:54
@6543
Copy link
Member

6543 commented May 28, 2020

OK, one thing left Tests

@tle-huu
Copy link
Contributor Author

tle-huu commented May 28, 2020

OK, one thing left Tests

I have trouble running the tests locally, getting an internal error related to pushing / pre-receive hooks (when I run make test-pgsql):

CreateRepoBranch: PushRejected Error: exit status 1: remote: Gitea: Internal Server Error
        	remote: Unable to contact gitea: Post "http://localhost:3002/api/internal/hook/pre-receive/user2/repo1": dial tcp [::1]:3002: connect: connection refused

EDIT: Ok looks like it is not just locally..

@tle-huu tle-huu force-pushed the terence/add_create_branch_to_api branch from ac34af1 to 7604857 Compare May 28, 2020 16:46
@6543
Copy link
Member

6543 commented May 28, 2020

the test itself looks good, so there is an issue with the CreateBranch implementation

@tle-huu
Copy link
Contributor Author

tle-huu commented May 28, 2020

Looks like it comes from an internal pre-receive hook (when I overwrite the hook in the gitea-repositories-meta repos it works), triggered because CreateNewBranch creates the new branch and then push it.

@6543
Copy link
Member

6543 commented May 28, 2020

Zeripaths Idear of creating a temporary repo sound a good solution for this ...

@tle-huu tle-huu force-pushed the terence/add_create_branch_to_api branch from 1e635de to 98245b4 Compare May 28, 2020 22:32
@@ -71,7 +71,9 @@ func CreateNewBranch(doer *models.User, repo *models.Repository, oldBranchName,
}

if !git.IsBranchExist(repo.RepoPath(), oldBranchName) {
return fmt.Errorf("OldBranch: %s does not exist. Cannot create new branch from this", oldBranchName)
return models.ErrBranchDoesNotExist{
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you checked every call to CreateNewBranch and made sure they handle your new error correctly?

Copy link
Contributor Author

@tle-huu tle-huu May 28, 2020

Choose a reason for hiding this comment

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

The other place where CreateNewBranch is used is in routers/repo/branch.go CreateBranch function and uses the ctx.Repo.BranchName as the "OldBranchName".

It does not "models.isErr" it but treats it as a ServerErr. Should we add a check there as well ? If so, should we do it in this PR or in another CL ?

@GiteaBot GiteaBot 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 May 29, 2020
Issue: go-gitea#11376

This commit introduces an API endpoint for branch creation.

The added route is POST /repos/{owner}/{repo}/branches.
A JSON with the name of the new branch and the name of the old branch is
required as parameters.

Signed-off-by: Terence Le Huu Phuong <[email protected]>
- Made the CreateNewBranch function return an errBranchDoesNotExist error
when the OldBranch does not exist
- Made the CreateBranch API function checks that the repository is not
empty and that branch exists.
fine-tune test env resetting
- Added api test for CreateBranch
- Used resetFixture instead of the more general prepareTestEnv in the
repo_branch_test CreateBranch tests
@tle-huu tle-huu force-pushed the terence/add_create_branch_to_api branch from 3758d73 to 9651305 Compare May 29, 2020 11:00
@tle-huu
Copy link
Contributor Author

tle-huu commented May 29, 2020

Sorry, I accidentally "merged" master into this branch.

I rebased on master (but this is equivalent to 5df39561490e79cb3237d541cf906c09eab8cfed)

@6543
Copy link
Member

6543 commented May 29, 2020

merging master int this (or hit the "update" button) is the normal way to let pulls be up-to-date
@tle-huu so dont worry

@zeripath
Copy link
Contributor

make lg-tm work

@zeripath zeripath merged commit 141d52c into go-gitea:master May 29, 2020
@6543
Copy link
Member

6543 commented May 29, 2020

ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
* [FEATURE] [API] Add Endpoint for Branch Creation

Issue: go-gitea#11376

This commit introduces an API endpoint for branch creation.

The added route is POST /repos/{owner}/{repo}/branches.
A JSON with the name of the new branch and the name of the old branch is
required as parameters.

Signed-off-by: Terence Le Huu Phuong <[email protected]>

* Put all the logic into CreateBranch and removed CreateRepoBranch

* - Added the error ErrBranchDoesNotExist in error.go
- Made the CreateNewBranch function return an errBranchDoesNotExist error
when the OldBranch does not exist
- Made the CreateBranch API function checks that the repository is not
empty and that branch exists.

* - Added a resetFixtures helper function in integration_test.go to
fine-tune test env resetting
- Added api test for CreateBranch
- Used resetFixture instead of the more general prepareTestEnv in the
repo_branch_test CreateBranch tests

* Moved the resetFixtures call inside the loop for APICreateBranch function

* Put the prepareTestEnv back in repo_branch_test

* fix import order/sort api branch test

Co-authored-by: zeripath <[email protected]>
@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/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them 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.

5 participants