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

Give user a link to create PR after push #4716

Merged
merged 5 commits into from Oct 20, 2018

Conversation

6 participants
@JulienTant
Copy link
Contributor

JulienTant commented Aug 15, 2018

New Feature

This PR offers a link to create new pull request when we push to a branch which is not the default branch.
If a PR already exists, it just give the link to the existing PR.

Output of a push when no active PR exists:

[branch2/test be40717] test
 1 file changed, 1 deletion(-)
Counting objects: 3, done.
Writing objects: 100% (3/3), 255 bytes | 255.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0)
remote:
remote: Create a new pull request for 'branch2/test':
remote:   http://localhost:3000/test1/test/compare/master...branch2%2Ftest
remote:
To http://localhost:3000/test1/test.git
   0c5683f..be40717  branch2/test -> branch2/test

Output of a push when an active PR exists:

[branch2/test 453d638] test
 1 file changed, 1 insertion(+)
Counting objects: 3, done.
Writing objects: 100% (3/3), 255 bytes | 255.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0)
remote:
remote: Visit the existing pull request:
remote:   http://localhost:3000/test1/test/pulls/6
remote:
To http://localhost:3000/test1/test.git
   be40717..453d638  branch2/test -> branch2/test
@JulienTant

This comment has been minimized.

Copy link
Contributor Author

JulienTant commented Aug 15, 2018

Now i wanna talk about some technical decision i took :

  1. In case of errors, I log but does nothing because I don't want this feature to block anything.
  2. I created 2 internal endpoints, but I though about an option where i could make only one that gave me everything I needed : the PR default branch + the PR if it exists.
  3. About the fork limitation, If you guys think it's better to create the PR in the "base" repository, then maybe it's better to consider to use what i described above to have the url generation out of the cmd/hook.go file

@bkcsoft bkcsoft added the lgtm/need 2 label Aug 15, 2018

@JulienTant JulienTant force-pushed the JulienTant:create-pr-after-push branch 3 times, most recently from 00520d7 to 37822e0 Aug 15, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 15, 2018

Codecov Report

Merging #4716 into master will decrease coverage by 0.02%.
The diff coverage is 28.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4716      +/-   ##
==========================================
- Coverage    37.4%   37.38%   -0.03%     
==========================================
  Files         307      308       +1     
  Lines       45545    45605      +60     
==========================================
+ Hits        17038    17050      +12     
- Misses      26052    26097      +45     
- Partials     2455     2458       +3
Impacted Files Coverage Δ
routers/private/internal.go 62.5% <100%> (+3.4%) ⬆️
routers/private/repository.go 25.86% <25.86%> (ø)
models/repo_indexer.go 50.84% <0%> (-1.28%) ⬇️
models/repo_list.go 62.2% <0%> (-1.17%) ⬇️

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 cabdf84...d7a6db3. Read the comment docs.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Aug 15, 2018

gitea_no_gcc should be removed.
I think the PR should be in base repository.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Aug 15, 2018

And you also should check if user has permission to visit and create pull request.

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Aug 15, 2018

I also agree that PR should be for base repository as that is default behaviour from UI

@JulienTant JulienTant force-pushed the JulienTant:create-pr-after-push branch from 37822e0 to 0fce8e1 Aug 15, 2018

@JulienTant JulienTant force-pushed the JulienTant:create-pr-after-push branch from 0fce8e1 to 965aa2a Aug 15, 2018

@JulienTant

This comment has been minimized.

Copy link
Contributor Author

JulienTant commented Aug 15, 2018

Ok I will first add the fork thing.

And you also should check if user has permission to visit and create pull request.

I'm not sure how to do that. I took a look at the "web" router to access to PR creation page and I found this :

// MustAllowPulls check if repository enable pull requests
func MustAllowPulls(ctx *context.Context) {
if !ctx.Repo.Repository.AllowsPulls() {
ctx.NotFound("MustAllowPulls", nil)
return
}
// User can send pull request if owns a forked repository.
if ctx.IsSigned && ctx.User.HasForkedRepo(ctx.Repo.Repository.ID) {
ctx.Repo.PullRequest.Allowed = true
ctx.Repo.PullRequest.HeadInfo = ctx.User.Name + ":" + ctx.Repo.BranchName
}
}

Is that what I must us implement ?

@JulienTant

This comment has been minimized.

Copy link
Contributor Author

JulienTant commented Aug 15, 2018

  • Forks are now created for the base repository.
  • Ensure that the "target" repository has enabled PR.

@lafriks lafriks added this to the 1.6.0 milestone Aug 15, 2018

@@ -0,0 +1,64 @@
package private

This comment has been minimized.

@lunny

lunny Aug 16, 2018

Member

comment head

This comment has been minimized.

@JulienTant

JulienTant Aug 16, 2018

Author Contributor

I changed my PHPStorm template for the project, thanks.

@JulienTant

This comment has been minimized.

Copy link
Contributor Author

JulienTant commented Aug 22, 2018

Hi guys, any news on this ?

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Aug 24, 2018

@JulienTant it looks good, nice work but I want to test it first before I give my approval

@JulienTant

This comment has been minimized.

Copy link
Contributor Author

JulienTant commented Aug 24, 2018

Of course. I tried to lot of cases but if you find anything i'll be happy to fix/change things.

@techknowlogick techknowlogick modified the milestones: 1.6.0, 1.7.0 Sep 3, 2018

@JulienTant

This comment has been minimized.

Copy link
Contributor Author

JulienTant commented Oct 1, 2018

up?

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Oct 1, 2018

We are feature freeze currently so need to wait until v1.6.0-rc1 is released

@lunny

lunny approved these changes Oct 20, 2018

@bkcsoft bkcsoft removed the lgtm/need 2 label Oct 20, 2018

@bkcsoft bkcsoft added the lgtm/need 1 label Oct 20, 2018

@lunny lunny added the changelog label Oct 20, 2018

@bkcsoft bkcsoft added lgtm/done and removed lgtm/need 1 labels Oct 20, 2018

@lafriks lafriks merged commit dea3d84 into go-gitea:master Oct 20, 2018

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

HoffmannP pushed a commit to HoffmannP/gitea that referenced this pull request Nov 14, 2018

Give user a link to create PR after push (go-gitea#4716)
* Give user a link to create PR after push

* Forks now create PR in the base repository + make sure PR creation is allowed

* fix code style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment