Revisiting API for creating PRs

[mfocko]

Current API appears to be fully functional and sufficiently covered by tests for GitHub and Pagure.

Services

GitHub + GitLab

• allows creating fork of forks
• allows creating PRs from one fork to another

Pagure

• doesn't allow creating fork of forks
• allows creating PRs on a project (assuming it's not a fork) or from fork to upstream

Current implementation

def create_pr(
  self,
  title: str,
  body: str,
  target_branch: str,
  source_branch: str,
  fork_username: str = None,
) -> "PullRequest":

Example objects used later

upstream_project = service.get_project(namespace="packit-service", repo="ogr")
my_fork = service.get_project(namespace="mfocko", repo="ogr")
fork_of_different_user = service.get_project(namespace="lachmanfrantisek", repo="ogr")

[mfocko]

Current behaviour

pull request from one branch to another on the upstream project

upstream_pr = upstream.create_pr(
  title="example title",
  body="pr description",
  target_branch="master",
  source_branch="my-patch-on-upstream"
)

pull request from fork to the upstream project

upstream_pr = my_fork.create_pr(
  title="example title",
  body="pr description",
  target_branch="master",
  source_branch="my-patch-on-my-fork"
)

pull request from one branch on my fork to another branch of my fork

pr_on_my_fork = my_fork.create_pr(
  title="example title",
  body="pr description",
  target_branch="my-wip",
  source_branch="my-patch",
  fork_username="mfocko" # or you can get username from service
)

pull request from one fork to another

pr_on_other_fork = my_fork.create_pr(
  title="example title",
  body="pr description",
  target_branch="patch-of-other-user",
  source_branch="my-patch-for-other-user",
  fork_username="lachmanfrantisek"
)

Summary

The way you can use create_pr may seem a bit counter-intuitive since you're
calling it on project in which the PR may not be created (e.g. fork -> upstream).

Examples above call create_pr on projects you're pulling from. There are two
side effects of implementation that allow it other way too.

Has a problem with forks of forks!

Created PRs keep back-reference to the project they've been called on, even though they may not be created on that project.

[mfocko]

Option A - Separate functions

create_pr behaviour would be constrained only to project it's being called on.
A new function create_pr_to_upstream would be introduced.

pull request from one branch to another on the upstream project

upstream_pr = upstream.create_pr(
  title="example title",
  body="pr description",
  target_branch="master",
  source_branch="my-patch-on-upstream"
)

pull request from fork to the upstream project

upstream_pr = my_fork.create_pr_to_upstream(
  title="example title",
  body="pr description",
  target_branch="master",
  source_branch="my-patch-on-my-fork"
)

pull request from one branch on my fork to another branch of my fork

pr_on_my_fork = my_fork.create_pr(
  title="example title",
  body="pr description",
  target_branch="my-wip",
  source_branch="my-patch"
)

pull request from one fork to another

pr_on_other_fork = fork_of_different_user.create_pr(
  title="example title",
  body="pr description",
  target_branch="patch-of-other-user",
  source_branch="mfocko:my-patch", # alternatively f"{username_from_service}:my-patch"
)

Summary

This approach would allow to keep flexibility of creating PRs against the upstream, but with explicitly named function, which would mean there's no monkey business going in the background implicitly.

For PR from one fork to another fork there is also a possibility of:

1. forcing user to prefix source branch with username (used in example)
2. keeping fork_username to handle it in method

[mfocko]

Option B - PRs are created on the project the method is called on

pull request from one branch to another on the upstream project

upstream_pr = upstream.create_pr(
  title="example title",
  body="pr description",
  target_branch="master",
  source_branch="my-patch-on-upstream"
)

pull request from fork to the upstream project

upstream_pr = upstream.create_pr( # alternatively `my_fork.parent.create_pr`
  title="example title",
  body="pr description",
  target_branch="master",
  source_branch="my-patch-on-my-fork",
  fork_username="mfocko" # alternatively call to service to acquire username
)

pull request from one branch on my fork to another branch of my fork

pr_on_my_fork = my_fork.create_pr(
  title="example title",
  body="pr description",
  target_branch="my-wip",
  source_branch="my-patch"
)

pull request from one fork to another

pr_on_other_fork = fork_of_different_user.create_pr(
  title="example title",
  body="pr description",
  target_branch="patch-of-other-user",
  source_branch="my-patch",
  fork_username="mfocko"
)

Summary

There could be problem with handling fork_username in two different contexts
(as it already is):

1. I have no parent -> traverses forks, OK
2. I am fork -> look for my forks? or forks of my parent?

Traversing through forks resolves problem with renamed forks. However
introduces problem with forks of forks.

On the other hand, this approach makes it explicit where the PR is being created.

[mfocko]

Option C - call on projects you're pulling from

Basically current behaviour, but wouldn't allow implicit PR to parent if called on fork.

• This approach has problems with resolution of project?
• Can I even know if the projects are in fork-parent or fork-fork or even fork-fork_of_fork relation?
• How could we handle the PRs from fork to upstream?
• How could we handle the PRs from one fork to another?

Could be same as option A, but other way around. (Requires multiple API calls, since we need to get internal projects.)

What about renaming? (Seems like github/gitlab uses username to resolve the fork and branch)

[mfocko]

Summary

• fork_username may be a problematic parameter, since it's used in 2 different contexts:
  • PRs on the fork
  • PRs from one fork to another
• which option favours the packit, packit-service and users the most?
  • which one would cause as less complications as possible in the code that uses it

[TomasTomecek]

Thank you for such a thorough write-up.

I personally like B

[lachmanfrantisek]

@mfocko thank you!

[csomh]

Under the hood I would imagine something like this:

create_pr(source_project, source_branch, target_project, target_branch)

This function could live outside of any class. Would be explicit.

Classes could have create_pr* convenience methods to deal with the fork/upstream aspects, but under the hood all would just call this function.

Would something like this work, or it's total non-sense?

[lachmanfrantisek]

@saisankargochhayat @cverna @thrix (and also @packit-service/the-packit-team)

Since you use OGR, you might be interested in this issue and the proposed way of creating pull-requests. (Or just be informed that we are thinking about changing the API a little.) Can you please take a look at this proposal and use 👍 and 👎 reactions on the previous comments with the proposals.

Thanks in advance!

[lachmanfrantisek]

Would something like this work, or it's total non-sense?

Yes, that makes sense to have this under the hood. Also, the defaults are here much clearer.

But still, we probably want to have some method(s) in the project class.

[stale]

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned, security, bug or EPIC are
never marked as stale.)

[stale]

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned, security, bug or EPIC are
never marked as stale.)

[lachmanfrantisek]

Looks like we have two votes for the second option and noone else cares about that.

@mfocko can we start working on this?

So:

• Implement the version B. I am for B as well but have one suggestion:
  • What about having also fork argument instead of the fork username, what do you think? Or instead of the username?
• Do what Hunor suggested under the hood.

[mfocko]

@lachmanfrantisek sure

[stale]

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned or EPIC are
never marked as stale.)

[stale]

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned or EPIC are
never marked as stale.)

[lachmanfrantisek]

@mfocko Do you have some time to take a look? (Some Friday work?..;-)

[stale]

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned or EPIC are
never marked as stale.)

[stale]

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned or EPIC are
never marked as stale.)

[lachmanfrantisek]

It would be really nice to finish this work.

[stale]

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned or EPIC are
never marked as stale.)

[mfocko]

🙄

[stale]

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned or EPIC are
never marked as stale.)

[lachmanfrantisek]

Another try?..;)

[mfocko]

Friday? for real now? :D I've also made changes in packit(|-service), so we should be able to remove the deprecated functions too… unless there are dependencies in stream and validation…

[stale]

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned or EPIC are
never marked as stale.)