Skip to content
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

doc: add dco to github pr template #24023

Closed
wants to merge 3 commits into from

Conversation

@MylesBorins
Copy link
Member

commented Nov 1, 2018

Our project relies on the Developer Certificate of Origin as a passive
alternative to a Contributor License Agreement. The PR template seems
like a great place to surface the text to help new contributors be
aware of the DCO and it's implications.

doc: add dco to github pr template
Our project relies on the Developer Certificate of Origin as a passive
alternative to a Contributor License Agreement. The PR template seems
like a great place to surface the text to help new contributors be
aware of the DCO and it's implications.

@MylesBorins MylesBorins requested a review from nodejs/tsc Nov 1, 2018

@mcollina
Copy link
Member

left a comment

LGTM

@@ -14,3 +14,32 @@ Contributors guide: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md
- [ ] tests and/or benchmarks are included
- [ ] documentation is changed or added
- [ ] commit message follows [commit guidelines](https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines)

<!--
<a id="developers-certificate-of-origin"></a>

This comment was marked as resolved.

Copy link
@richardlau

richardlau Nov 1, 2018

Member

This anchor doesn't need to be here?

This comment was marked as resolved.

Copy link
@MylesBorins

MylesBorins Nov 1, 2018

Author Member

Done, this was copy pasted from how the DCO is included in other places. Makes sense to remove it

@targos
targos approved these changes Nov 1, 2018
@lpinca
lpinca approved these changes Nov 1, 2018
@cjihrig
cjihrig approved these changes Nov 1, 2018
@richardlau
Copy link
Member

left a comment

Maybe also add an item that can be checked under the checklist?

@Trott

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

This nearly triples the length of our PR template, increasing the likelihood that people will not bother reading this or any of the other text already there. Not blocking, but options I would prefer instead of this:

  • a checkbox in the template indicating that the user has read and understood the DCO text in CONTRIBUTING.md (so no need to put all the text here)
  • replace the DCO text in CONTRIBUTING.md with a link or other reference to this newly-added text in the template (so that we don't have the text duplicated in two places)

Options I'd be OK with that are likely to be even less popular:

  • no change; the DCO is already in CONTRIBUTING.md
  • removal of the GitHub templates; they seem to get in the way more than they help

In general, I think we should have fewer templates and shorter templates. DCO text is like a click-through license. It's a necessary evil that gets in the way and no one can take it seriously as something people read and agree to. If we have to add it here, oh well, so be it. Although I wonder what a lawyer's take would be....

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2018

@Trott we could definitely add it to the checklist instead with a link people could click through

Also FWIW I was thinking that a DCO bot is something we could look at using... a bot that posts in the thread anytime someone is a new collaborator (doesn't have a commit in the repo they are opening a PR to)

Would you be open to landing this or an iteration and then removing it when a bot like mentioned above is launched?

@refack

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Options I'd be OK with that are likely to be even less popular:

  • no change; the DCO is already in CONTRIBUTING.md
  • removal of the GitHub templates; they seem to get in the way more than they help

Option 3:
Since the template is an informal guide I'd suggest adding a single sentence at the header jest before:

By submitting you agree to this project's Developer's Certificate of Origin in the

i.e.

<!--
Thank you for your pull request. Please provide a description above and review
the requirements below.

Bug fixes and new features should include tests and possibly benchmarks.

By submitting you agree to this project's Developer's Certificate of Origin in the
Contributors guide: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md
-->
@Trott

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Would you be open to landing this or an iteration and then removing it when a bot like mentioned above is launched?

@MylesBorins Yes. My comments are non-blocking, as this can always be iterated on.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Nov 4, 2018

Gonna think about this some more and reopen later

@MylesBorins MylesBorins closed this Nov 4, 2018

@MylesBorins MylesBorins reopened this Aug 23, 2019

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

Hey All,

I'd like to revisit this. It had a bunch of sign off. Any objections to moving forward with this?

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Have there been problems that this solves? It adds a lot of text to the (IMO) already-ineffective GitHub templates.... People often ignore the checkboxes. People often check the box saying they ran make test when they clearly did not. And so on....

I'm not blocking on this, but I do think it would be better to remove the templates or shorten the templates. If we're going to add a bunch of text, let's be very clear about the value this adds.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Have there been problems that this solves? It adds a lot of text to the (IMO) already-ineffective GitHub templates.... People often ignore the checkboxes. People often check the box saying they ran make test when they clearly did not. And so on....

I'm not blocking on this, but I do think it would be better to remove the templates or shorten the templates. If we're going to add a bunch of text, let's be very clear about the value this adds.

You don't need to respond to this comment before landing. My questions can be dealt with later or not at all.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

@Trott as per the original post

Our project relies on the Developer Certificate of Origin as a passive
alternative to a Contributor License Agreement. The PR template seems
like a great place to surface the text to help new contributors be
aware of the DCO and it's implications.

While this adds cruft I do think it is important to surface this text to folks as it is the terms of their contribution

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

While this adds cruft I do think it is important to surface this text to folks as it is the terms of their contribution

Works for me. It's not my preference, but I don't oppose it, and also I am not a lawyer and can't speak to DCO issues competently anyway. So I defer to the apparent overwhelming support this has!

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Hmmm... lite CI again but this time with explicit rebase-onto-master: https://ci.nodejs.org/job/node-linter/3788/

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Landed in a9512bd

@Trott Trott closed this Aug 26, 2019

Trott added a commit to Trott/io.js that referenced this pull request Aug 26, 2019
doc: add dco to github pr template
Our project relies on the Developer Certificate of Origin as a passive
alternative to a Contributor License Agreement. The PR template seems
like a great place to surface the text to help new contributors be
aware of the DCO and it's implications.

PR-URL: nodejs#24023
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR added a commit that referenced this pull request Sep 3, 2019
doc: add dco to github pr template
Our project relies on the Developer Certificate of Origin as a passive
alternative to a Contributor License Agreement. The PR template seems
like a great place to surface the text to help new contributors be
aware of the DCO and it's implications.

PR-URL: #24023
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BridgeAR BridgeAR referenced this pull request Sep 3, 2019
BridgeAR added a commit that referenced this pull request Sep 4, 2019
doc: add dco to github pr template
Our project relies on the Developer Certificate of Origin as a passive
alternative to a Contributor License Agreement. The PR template seems
like a great place to surface the text to help new contributors be
aware of the DCO and it's implications.

PR-URL: #24023
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
doc: add dco to github pr template
Our project relies on the Developer Certificate of Origin as a passive
alternative to a Contributor License Agreement. The PR template seems
like a great place to surface the text to help new contributors be
aware of the DCO and it's implications.

PR-URL: nodejs#24023
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
doc: add dco to github pr template
Our project relies on the Developer Certificate of Origin as a passive
alternative to a Contributor License Agreement. The PR template seems
like a great place to surface the text to help new contributors be
aware of the DCO and it's implications.

PR-URL: nodejs#24023
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.