Skip to content

Conversation

@karlhungus
Copy link
Contributor

The good:

  • You can do a merge request for a forked commit and it will merge properly (i.e. it does work).
  • Push events take into account merge requests on forked projects
  • Tests around merge_actions now present, spinach, and other rspec tests

The questionable:

  • Events only know about target projects
  • Project's merge requests only hold on to MR's where they are the target
  • All operations performed in the satellite

The bad:

  • Satellites always always recreate
  • Duplication between project's repositories and satellites (e.g. commits_between)

(for reference: http://feedback.gitlab.com/forums/176466-general/suggestions/3456722-merge-requests-between-projects-repos)

Change-Id: I5502883b8b57df7fe16dc7253db20e5ca7b7f702

The good:

 - You can do a merge request for a forked commit and it will merge properly (i.e. it does work).
 - Push events take into account merge requests on forked projects
 - Tests around merge_actions now present, spinach, and other rspec tests

The questionable:

 - Events only know about target projects
 - Project's merge requests only hold on to MR's where they are the target
 - All operations performed in the satellite

The bad:

  - Satellites always always recreate
  -  Duplication between project's repositories and satellites (e.g. commits_between)

(for reference: http://feedback.gitlab.com/forums/176466-general/suggestions/3456722-merge-requests-between-projects-repos)

Change-Id: I5502883b8b57df7fe16dc7253db20e5ca7b7f702
@karlhungus
Copy link
Contributor Author

So this one I've fixed some of the things i mentioned before and added some tests around merge_action and action.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling e99c0a7 on karlhungus:mr-on-fork into 76ffd11 on gitlabhq:master.

@amacarthur
Copy link
Contributor

+1

By allowing you to submit merge requests cross-repo back to the fork origin, this should close the loop on forking workflow.

See #3597

@karlhungus
Copy link
Contributor Author

Little bit surprised that coverage remained the same -- i expected with the new tests around merge_action the coverage would go up.

@karlhungus
Copy link
Contributor Author

Specifically addresses: #3597 (comment)

@karlhungus
Copy link
Contributor Author

Tests run pretty slow with all the additional satellite creation, looking into preventing these from running before every test.

@PaulusTM
Copy link

👍 Nice feature! This morning I've tried if I could fork and than make a pull request but sadly that was not possible.

@karlhungus
Copy link
Contributor Author

@MasterExplosive -- just checking -- you didn't find an issue with this PR did you?

-Spinach/Rspec now only initialize test directory, and setup stubs (things that are relatively cheap)
-project_with_code, source_project_with_code, and target_project_with_code now create/destroy their repos individually

fixed remote removal

Change-Id: I01b0e1b8f9ba1200c4ad58bfea132af0e499320b
@karlhungus
Copy link
Contributor Author

This should speed up the test satellite creation quite a bit.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling b125fc4 on karlhungus:mr-on-fork into 76ffd11 on gitlabhq:master.

Change-Id: Iaa13c37f4cfd01083e2a5c297cc06896d84659d1
@karlhungus
Copy link
Contributor Author

It seems i completely broke the travis build with that last one, this should fix it (our own ci server -- jenkins must be slightly more lazy).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 75ced76 on karlhungus:mr-on-fork into 76ffd11 on gitlabhq:master.

@karlhungus
Copy link
Contributor Author

Looking into that failing NotificationService test now, had enabled some observers, and didn't disable them afterwards

Change-Id: Ic1c2764e23e07195f0d69a5dcaec394001bf1c3e
@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling 5cc5ea4 on karlhungus:mr-on-fork into 76ffd11 on gitlabhq:master.

@karlhungus
Copy link
Contributor Author

Added some comments from our own code review system - will take care of these as i get to them.

@karlhungus
Copy link
Contributor Author

Additionally found errors with editing merge requests, and retrieving diff's on forked MR's

@MrKeiKun
Copy link
Contributor

MrKeiKun commented Jun 1, 2013

will this pull request implement the updating of the project you forked from the original one?
that feature will be helpful.

@karlhungus
Copy link
Contributor Author

@shidokun this functionality works now (there are issues, but it basically works; i.e. if you want you can test it yourself).

You mean making merge requests to projects that have forked from you? It doesn't support that right now, but typically as the manager of a fork you'd be a consumer of the upstream, and be pulling and updating manually. This is something that could be done eventually, but i'd find it's value a little hard to justify, because at that point (which seems like a bit of an edge case) you can just fork the fork, and start making merge requests on that.

Regardless of my opinion, it doesn't do the above currently, but adding that kind of functionality after this change would be much easier.

@MrKeiKun
Copy link
Contributor

MrKeiKun commented Jun 1, 2013

is it under master? currently im using 5.2 though

i mean example
i fork gitlabhq and now i own it "ShidoKun/gitlabhq"
what if i want ShidoKun/gitlabhq get the latest changes from
gitlabhq/gitlabhq

i was finding that feature and i think its called pull request...
and another thing does this feature = pull request to gitlab from github?
cause i remember that gitlab has this feature creating project using import
i checked how it works though i checked project settings and other features
i can't my project update from github to my gitlab.. i think its same term like git pull request too..
in shorts its like fork update of your project from the original one

@karlhungus
Copy link
Contributor Author

You would have to pull it yourself -- it's still very much in "beta". I've got some more patches for it coming. It really just allows for merge requests on projects you've forked -- just like git hub. I do like your ideas I think they need this function first tough.

@MrKeiKun
Copy link
Contributor

MrKeiKun commented Jun 1, 2013

though i remember..
github already has this feature
updating the project you fork from its origin

though its better if will be merge function with the merge request
including the pull request from repository to another one

@karlhungus
Copy link
Contributor Author

I can't find that function on github, and always update my github repos by deleteing the branch i want to update and pushing the latest there. How are you doing this?

In anycase this is a different problem.

@karlhungus
Copy link
Contributor Author

I'm going to close this and reopen it with all the fixes, rather then apply the fixes individually, hopefully this makes life easier for the mergers.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants