diff mbox series

github: update pull request template

Message ID 20200604035322.731006-1-oohall@gmail.com
State Accepted
Headers show
Series github: update pull request template | expand

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (6bf21350da32776aac8ba75bf48933854647bd7e)

Commit Message

Oliver O'Halloran June 4, 2020, 3:53 a.m. UTC
The current wording is a bit curt. Flesh it out a bit and put in some
useful detail.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
I guess the real question we should be asking is: Why don't we take
github pull requests? The best answer I can offer is that it seems to
be impossible to get the DCO (the signed-off-by, etc) correct when using
PRs since github doesn't let you screw with the patches proper when
merging a PR. As far as I can tell anyway, I could be wrong.
---
 .github/pull_request_template.md | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Comments

Andrew Donnellan June 4, 2020, 5:11 a.m. UTC | #1
On 4/6/20 1:53 pm, Oliver O'Halloran wrote:
> The current wording is a bit curt. Flesh it out a bit and put in some
> useful detail.
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

New message looks good.

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>

> ---
> I guess the real question we should be asking is: Why don't we take
> github pull requests? The best answer I can offer is that it seems to
> be impossible to get the DCO (the signed-off-by, etc) correct when using
> PRs since github doesn't let you screw with the patches proper when
> merging a PR. As far as I can tell anyway, I could be wrong.

The DCO issue is addressable by a CI bot that tells you off for screwing 
that up, and then abandoning the idea that the maintainer who commits it 
has to add their own SOB. Though there are other cases where maintainers 
want to fix up commit messages or minor fixes during merge which aren't 
as practical with GitHub.

The bigger issue to me is that I think it's best if the project has one 
path for submitting patches. It may be better to be all-or-nothing and 
switch to GitHub completely if it suits our needs, I'm less keen on 
having 70% of patches go through the mailing list and 30% go through GitHub.
Vasant Hegde June 4, 2020, 5:51 a.m. UTC | #2
On 6/4/20 9:23 AM, Oliver O'Halloran wrote:
> The current wording is a bit curt. Flesh it out a bit and put in some
> useful detail.
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> I guess the real question we should be asking is: Why don't we take
> github pull requests? The best answer I can offer is that it seems to
> be impossible to get the DCO (the signed-off-by, etc) correct when using
> PRs since github doesn't let you screw with the patches proper when
> merging a PR. As far as I can tell anyway, I could be wrong.

Looks good to me.

Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>


-Vasant


> ---
>   .github/pull_request_template.md | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
> index bc7d9614fb5a..ce97aa6b0676 100644
> --- a/.github/pull_request_template.md
> +++ b/.github/pull_request_template.md
> @@ -1,3 +1,18 @@
> -Please do not submit a Pull Request via github.  Our project makes use of
> -mailing lists for patch submission and review.  For more details please
> -see https://open-power.github.io/skiboot/
> +Hi there,
> +
> +The skiboot project (generally) doesn't take contributions via github pull
> +requests. Contributions should sent to the skiboot mailing list using the
> +git-format-patch and git-send-email tools. If you're unfamiliar with these
> +tools, there's a guide here:
> +
> +https://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/
> +
> +The list address is:
> +
> +	skiboot@lists.ozlabs.org
> +
> +Posting to the list does not require subscribing to the list. Posts from a
> +non-subscribers are moderated to cut down on spam, but anyone posting
> +genuine contributions will be whitelisted.
> +
> +See https://open-power.github.io/skiboot/ for more details.
>
Klaus Heinrich Kiwi June 4, 2020, 2:08 p.m. UTC | #3
On 6/4/2020 2:11 AM, Andrew Donnellan wrote:
> On 4/6/20 1:53 pm, Oliver O'Halloran wrote:
>> The current wording is a bit curt. Flesh it out a bit and put in some
>> useful detail.
>>
>> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> 
> New message looks good.
> 
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
Reviewed-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
> 
>> ---
>> I guess the real question we should be asking is: Why don't we take
>> github pull requests? The best answer I can offer is that it seems to
>> be impossible to get the DCO (the signed-off-by, etc) correct when using
>> PRs since github doesn't let you screw with the patches proper when
>> merging a PR. As far as I can tell anyway, I could be wrong.
> 
> The DCO issue is addressable by a CI bot that tells you off for screwing that up, and then abandoning the idea that the maintainer who commits it has to add their own SOB. Though there are other cases where maintainers want to fix up commit messages or minor fixes during merge which aren't as practical with GitHub.

I believe Github by default allow changes to forked PR branches by the upstream repo maintainers: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork
So if the submitter hasn't explicitly disabled it, I believe maintainers are able to "fix" the PR in whatever ways necessary before applying the commit.

If it's a simple patch (no multi-part), I will usually opt to "rebase and merge" (which is a --ff merge) instead of adding a merge commit. If it's multi-part and a merge is in order, the maintainer will always have the opportunity to manually add it in it's own merge commit message, but I wonder if it's really necessary, considering the actual content being committed should already have the SOBs enforced from the DCO bot check (when enabled).

> 
> The bigger issue to me is that I think it's best if the project has one path for submitting patches. It may be better to be all-or-nothing and switch to GitHub completely if it suits our needs, I'm less keen on having 70% of patches go through the mailing list and 30% go through GitHub.

+1 here. I'll admit I actually like the Github-workflow better than mailing-list + patchwork. Nowadays we have a plethora of bots that could act on PRs (travis included, which should be available for ppc64le targets) that could be helpful, not mentioning the overall integration with reviews, issues etc. The downside is that conversation will probably move to github PRs and issues, something anyone interested would have to subscribe to and monitor if they want to be part of the conversation (in that sense not much different from mailing-lists).

  -Klaus
Oliver O'Halloran June 5, 2020, 4:33 a.m. UTC | #4
On Fri, Jun 5, 2020 at 12:08 AM Klaus Heinrich Kiwi
<klaus@linux.vnet.ibm.com> wrote:
>
> On 6/4/2020 2:11 AM, Andrew Donnellan wrote:
> > On 4/6/20 1:53 pm, Oliver O'Halloran wrote:
> >> The current wording is a bit curt. Flesh it out a bit and put in some
> >> useful detail.
> >>
> >> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> >
> > New message looks good.
> >
> > Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
> Reviewed-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
> >
> >> ---
> >> I guess the real question we should be asking is: Why don't we take
> >> github pull requests? The best answer I can offer is that it seems to
> >> be impossible to get the DCO (the signed-off-by, etc) correct when using
> >> PRs since github doesn't let you screw with the patches proper when
> >> merging a PR. As far as I can tell anyway, I could be wrong.
> >
> > The DCO issue is addressable by a CI bot that tells you off for screwing that up, and then abandoning the idea that the maintainer who commits it has to add their own SOB. Though there are other cases where maintainers want to fix up commit messages or minor fixes during merge which aren't as practical with GitHub.
>
> I believe Github by default allow changes to forked PR branches by the upstream repo maintainers: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork
> So if the submitter hasn't explicitly disabled it, I believe maintainers are able to "fix" the PR in whatever ways necessary before applying the commit.

That works I guess.

> If it's a simple patch (no multi-part), I will usually opt to "rebase and merge" (which is a --ff merge) instead of adding a merge commit. If it's multi-part and a merge is in order, the maintainer will always have the opportunity to manually add it in it's own merge commit message, but I wonder if it's really necessary, considering the actual content being committed should already have the SOBs enforced from the DCO bot check (when enabled).

Honestly I don't care that much about s-o-b since we don't have the
hierarchy-of-maintainers problem that the kernel has. The git
committer / author name usually provides enough traceability for a
project like Skiboot. Yes, it does fall over a bit when you have
patches with multiple authors, but that's a pretty obscure problem.
The main problem I see is that the DCO block contains a list of
everyone involved in a patch as a reviewer, tester, etc which is
useful to have and we'd lose that entirely. It's not perfect, but it's
nice to have that sort of metadata attached to the commit itself
rather than stored externally.

> > The bigger issue to me is that I think it's best if the project has one path for submitting patches. It may be better to be all-or-nothing and switch to GitHub completely if it suits our needs, I'm less keen on having 70% of patches go through the mailing list and 30% go through GitHub.
>
> +1 here. I'll admit I actually like the Github-workflow better than mailing-list + patchwork. Nowadays we have a plethora of bots that could act on PRs (travis included, which should be available for ppc64le targets) that could be helpful, not mentioning the overall integration with reviews, issues etc. The downside is that conversation will probably move to github PRs and issues, something anyone interested would have to subscribe to and monitor if they want to be part of the conversation (in that sense not much different from mailing-lists).

I've always found the github interface clunky at the best of times,
but the same can be said about patchwork. There is finally a decent
github CLI tool (https://github.com/cli/cli/) so moving to GH might
actually be an improvement overall.

Oliver
Oliver O'Halloran June 5, 2020, 6:45 a.m. UTC | #5
On Thu, Jun 4, 2020 at 1:55 PM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> The current wording is a bit curt. Flesh it out a bit and put in some
> useful detail.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Merged as 5b1d49de9deb38ea668ccf1f822299f769f57a1a
diff mbox series

Patch

diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
index bc7d9614fb5a..ce97aa6b0676 100644
--- a/.github/pull_request_template.md
+++ b/.github/pull_request_template.md
@@ -1,3 +1,18 @@ 
-Please do not submit a Pull Request via github.  Our project makes use of
-mailing lists for patch submission and review.  For more details please
-see https://open-power.github.io/skiboot/
+Hi there,
+
+The skiboot project (generally) doesn't take contributions via github pull
+requests. Contributions should sent to the skiboot mailing list using the
+git-format-patch and git-send-email tools. If you're unfamiliar with these
+tools, there's a guide here:
+
+https://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/
+
+The list address is:
+
+	skiboot@lists.ozlabs.org
+
+Posting to the list does not require subscribing to the list. Posts from a
+non-subscribers are moderated to cut down on spam, but anyone posting
+genuine contributions will be whitelisted.
+
+See https://open-power.github.io/skiboot/ for more details.