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

Add Developer Certificate of Origin (DCO) #1481

Merged
merged 2 commits into from
Sep 22, 2016
Merged

Conversation

schiessle
Copy link
Member

As discussed with @karlitschek this PR adds a "Developer Certificate of Origin" (DCO) and a description how to apply it.

Basically all commits need to be signed with signed-off-by: your name <your email address>. git commit -s already does this for you.

Additionally I would add a bot which checks all commits of a PR if it contains this "signed-off-by" line.

cc @jospoortvliet to review the textual changes to CONTRIBUTING.md

@schiessle schiessle added enhancement 3. to review Waiting for reviews labels Sep 21, 2016
@mention-bot
Copy link

@schiessle, thanks for your PR! By analyzing the annotation information on this pull request, we identified @LukasReschke, @MariusBluem and @MorrisJobke to be potential reviewers

@karlitschek
Copy link
Member

nice 👍

@MariusBluem
Copy link
Member

👍 I've already done this on several PRs 😉

@LukasReschke
Copy link
Member

Additionally I would add a bot which checks all commits of a PR if it contains this "signed-off-by" line.

Let's wait with the merge until this is in. See .drone.yml on how to do that. Basically copy and adjust

server/.drone.yml

Lines 19 to 28 in e9780b7

app-check-code:
image: nextcloudci/php7.0:php7.0-2
commands:
- ./occ app:check-code admin_audit
- ./occ app:check-code comments
- ./occ app:check-code federation
- ./occ app:check-code workflowengine
when:
matrix:
TESTS: app-check-code
and
- TESTS: app-check-code

@@ -0,0 +1,33 @@
To improve tracking of who did what, we use the "sign-off" procedure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good explanation. This is what the git commit history is for. I'd rather start with the actual reasoning that this is for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not from me. That's the from https://github.com/wking/signed-off-by and more or less the default way to do it.

But let's see what I can do.

reviewer and means that she is completely satisfied that the patch
is ready for application. It is usually offered only after a
detailed review.
4. "Tested-by:" is used to indicate that the person applied the patch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suuuuper complicated stuff above and I doubt it will ever be used as it implies rewriting the history. ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't want to rewrite history... We want to use it from now on

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But NOBODY will use Tested-by orAcked-by and so on after a PR has been reviewed (as it requires rewriting the branch history).

This is superfluos information that just confuses user. See https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work on how to make it smaller.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just a "you can if you want". if nobody want to... fine. Anyway I just removed it. I don't care about it. It doesn't hurt if someone add extra tags but it also doesn't hurt if we don't mention it.

@schiessle schiessle force-pushed the signed-off-by branch 2 times, most recently from 7160353 to 7251ab8 Compare September 21, 2016 20:25
````

using your real name (sorry, no pseudonyms or anonymous
contributions). This line can be automatically added by git if you
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of that. But well… whatever…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that with the ownCloud CLA you can easily commit anonymous or with pseudonyms by just stating the contribution to be MIT licensed.

Copy link
Member Author

@schiessle schiessle Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to have something legally valid you need a real name. A "yes I have the right to give it to you under the AGPL" by SuperHacker83 doesn't make any sense

@schiessle
Copy link
Member Author

Simplified as much as possible. Yes, this adds some additional overhead for contributors. But the overhead is quite small and it improves the legal certainty, that's why we want to have it.

@LukasReschke
Copy link
Member

LukasReschke commented Sep 21, 2016

Fair enough. No idea about it to be honest but let me add a CI entry.

That said:

👎, your commit doesn't include the DCO. 😉

@schiessle
Copy link
Member Author

but let me add a CI entry.

Thanks!

your commit doesn't include the DCO. 😉

At the moment the contributor guidelines doesn't say that it have to... Anyway, just for Lukasli... 😉 

@LukasReschke LukasReschke force-pushed the signed-off-by branch 2 times, most recently from 0c22392 to c617a17 Compare September 21, 2016 23:09
@LukasReschke
Copy link
Member

👍 , added the checker to drone

@LukasReschke LukasReschke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 21, 2016
@schiessle schiessle force-pushed the signed-off-by branch 3 times, most recently from f8de660 to 6de9140 Compare September 22, 2016 07:05
schiessle and others added 2 commits September 22, 2016 09:08
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@LukasReschke LukasReschke merged commit 538fcf3 into master Sep 22, 2016
@LukasReschke LukasReschke deleted the signed-off-by branch September 22, 2016 08:55
@blizzz
Copy link
Member

blizzz commented Sep 26, 2016

Yes, this adds some additional overhead for contributors. But the overhead is quite small and it improves the legal certainty, that's why we want to have it.

Isn't the overhead that you as contributor need to verify whether you hurt a random patent?

@oparoz
Copy link
Member

oparoz commented Sep 28, 2016

FYI, there is zero overhead with the new PHPStorm. Just tick the box and your commits are signed.

@blizzz
Copy link
Member

blizzz commented Sep 29, 2016

Adding the -s to git commit is easily possible in multiple ways, but this is not my concern. It's a legal-related question.

@oparoz
Copy link
Member

oparoz commented Sep 29, 2016

Sorry, I wasn't replying to you directly. I just wanted to mention that it's now easily possible to do without having to create aliases and use alternative git commands :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants