Honor delete branch on merge repo setting when using merge API #35488
Conversation
GiteaBot
added
the
lgtm/need 2
github-actions
bot
added
modifies/api
|
The logic has been implemented, but for tests I need to figure out how to work with the git database in the test environment. Did not see issues when manual-testing the following:
|
|
Update: Will get to adding the tests, just been busy. |
|
I already written the API integration tests, just having problems with the test repos. I need to use the git hooks in However, I am getting "pre-receive hook declined" when the test code attempts to push back the changes to the original repo from the temp repo that is created by Gitea. Looking for solutions. |
|
Created some integration tests covering some of the merge request API. One thing I realized is that my changes will delete the branch after merge even if delete_branch_after_merge is not specified as a form field. I think the form field should take precedence over the repo setting equivalent. If the form field is not defined, we turn to the repo settings to make a determination. Though, the form field is defined as a |
GiteaBot
added
lgtm/need 1
tests/integration/api_pull_test.go
Outdated
| }) | ||
| } | ||
|
|
||
| func creatPullRequestWithCommit(t *testing.T, user *user_model.User, token, newBranch string, repo *repo_model.Repository) *api.PullRequest { |
There was a problem hiding this comment.
Duplicates:
- createFileInBranch
- doAPICreatePullRequest
There was a problem hiding this comment.
da4bb40 uses doAPICreatePullRequest(), but I don't think I can use createFileInBranch() as it won't allow me to create a new branch
There was a problem hiding this comment.
It can be done with some small changes 64db368
tests/integration/api_pull_test.go
Outdated
| MakeRequest(t, req, http.StatusOK) | ||
| doCheckBranchExists(t, owner, token, newBranch, repo, http.StatusOK) | ||
| // make sure we cannot perform a merge on the same PR | ||
| MakeRequest(t, req, http.StatusUnprocessableEntity) |
There was a problem hiding this comment.
This test is not right. StatusUnprocessableEntity is caused by reused req, it is an error caused by client, but not the "already merged" error.
The "already merged" error code is 405 (well, it is abused ... but old code does so)
routers/api/v1/repo/pull.go
Outdated
| // for agit flow, we should not delete the agit reference after merge | ||
| if form.DeleteBranchAfterMerge && pr.Flow == issues_model.PullRequestFlowGithub { | ||
| // FIXME: old code has that comment above. Is that comment valid? What would go wrong if a agit branch is deleted after merge? | ||
| // * If a agit branch can be deleted after merge, then fix the comment and maybe other related code | ||
| // * If a agit branch should not be deleted, then we need to fix the logic and add more tests |
There was a problem hiding this comment.
Now we have a new question, I think it needs to be addressed.
Maybe @lunny @a1012112796 have some ideas
There was a problem hiding this comment.
I answered myself by rewriting the old code, and fixed more legacy problems together.
wxiaoguang
force-pushed
the
fix/inherit-merge-api-delete-branch-repo-settings
branch
2 times, most recently
from
eeaef77 to
8438de6
Compare
github-actions
bot
added
the
modifies/templates
wxiaoguang
force-pushed
the
fix/inherit-merge-api-delete-branch-repo-settings
branch
from
8438de6 to
a68474a
Compare
GiteaBot
added
lgtm/done
4 participants
Fix #35463.