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

meta: add requirement for signed-off-by statements #31976

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 27, 2020

To improve handling of DCO compliance, the TSC is considering
the need to make inclusion of Signed-off-by statements in
every commit mandatory. The TSC is still working through the
process changes necessary to make this as painless as possible
but drafting the basic requirement is step one.

The Signed-off-by statement is an attestation that the commit
is contributed in accordance to the DCO.

This is a draft PR for now while the @nodejs/tsc deliberates further.

/cc @nodejs/tsc

/cc @nodejs/automation ... If this moves forward, then node-core-utils will need to be updated to check for, and possibly automatically apply, the Signed-off-by statements when preparing PRs to land.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 27, 2020
@mmarchini
Copy link
Contributor

If this moves forward, then node-core-utils will need to be updated to check for, and possibly automatically apply, the Signed-off-by statements when preparing PRs to land.

Maybe this should be checked by a linter on GH Actions instead? Also, if the goal is for an explicit acknowledge that the PR author is in accordance with DCO, it should be added by the committer, not by whoever lands the PR, so ncu should not apply it automatically.

@jasnell
Copy link
Member Author

jasnell commented Feb 27, 2020

Also, if the goal is for an explicit acknowledge that the PR author is in accordance with DCO, it should be added by the committer, not by whoever lands the PR, so ncu should not apply it automatically.

What the @nodejs/tsc has discussed is the option of allowing the contributor to signal in the discussion thread that they acknowledge and agree to the contribution under the DCO. If that works, then the Signed-off-by can be added by anyone. The goal would be to make it as simple as possible for brand new or "drive-by" contributors. That said, you're correct, ideally the Signed-off-by would be added by the contributor themselves in the commit.

@richardlau
Copy link
Member

For automation we could have add a label (e.g, signed-off) which would be either automatically added by a GitHub action workflow (e.g. if the commits were signed off) or manually added by a collaborator if the sign-off was indicated in the discussion thread.

node-core-utils could then look for the signed-off label, error if it’s not there or automatically apply any missing Signed-off-by if it is present.

COLLABORATOR_GUIDE.md Outdated Show resolved Hide resolved
COLLABORATOR_GUIDE.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
COLLABORATOR_GUIDE.md Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM with the formatting changes reverted.

To improve handling of DCO compliance, the TSC is considering
the need to make inclusion of `Signed-off-by` statements in
every commit mandatory. The TSC is still working through the
process changes necessary to make this as painless as possible
but drafting the basic requirement is step one.

The Signed-off-by statement is an attestation that the commit
is contributed in accordance to the DCO.

Signed-off-by: James M Snell <[email protected]>
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@JasonEtco
Copy link

JasonEtco commented Feb 27, 2020

👋 Wanted to mention that @MylesBorins and I were chatting about this back in Montreal, so I spiked out this action. It allows contributors to write /signoff in a PR comment and create a commit with an explicit sign-off message.

I haven't been able to get back to it since, I'm happy to transfer the repo to the Node.js org if y'all are interesting in taking it over. It could be adapted to rebase/amend commits in a similar way to retroactively signoff.

@mmarchini
Copy link
Contributor

Wanted to mention that @MylesBorins and I were chatting about this back in Montreal, so I spiked out this action. It allows contributors to write /signoff in a PR comment and create a commit with an explicit sign-off message.

That looks great! Have you checked if it works with pull requests coming from a fork? GitHub Actions has permission restrictions for Pull Requests from forks.

@brianwarner
Copy link

brianwarner commented Mar 6, 2020

Hi all, @MylesBorins pointed me at this PR.

The best solution we've found at the LF is Probot DCO, which checks for signoffs. It passes commits which are properly signed off, and blocks merging for those which aren't (with the option to override, if so configured).

I've been working on extending Probot DCO over the past week to recognize remediation commits - e.g., a subsequent commit in the branch which includes specific text signing off on the commit which didn't have it. This is almost ready, probably just need another good weekend of work, and then I'll submit it upstream for inclusion.

This works great if you can push a new commit with remediation text, but we still have the gap where you have to have a local git environment in order to do it. @JasonEtco , Myles pointed me at your action. I think this is the right idea, and we might want to look at incorporating the idea into Probot DCO so it comes in one installable app. I spoke with Bex (Probot DCO maintainer), and they're interested.

I'd suggest we link up some time next week. I'd like to get the remediation commit feature finished and submitted to Bex, and then perhaps we can work on adding the function to trigger adding a remediation commit via comment?

@mmarchini
Copy link
Contributor

If we decide to add automated signing-off commits with GitHub Actions, it's a good idea to coordinate with nodejs/build#2201, otherwise we'll have two actions trying to edit the Pull Request, which could lead to undefined behavior.

@brianwarner
Copy link

So just to be clear, the signoffs are not automated. They must be initiated by someone with the right to signoff on the commit. The only part that would be automated would be detecting magic text in the comments on the PR, which would then add a new commit to the branch.

@JasonEtco
Copy link

Have you checked if it works with pull requests coming from a fork? GitHub Actions has permission restrictions for Pull Requests from forks.

@mmarchini good question! I haven't specifically tried it, but (despite forks being generally different) I don't know of any reason this shouldn't work on a fork. It's setup to push to the branch/repo that the PR is based on, so in that case it'd be the fork.

I spoke with Bex (Probot DCO maintainer), and they're interested.

Hi @brianwarner 👋 that plan sounds great to me - always happy to work with @hiimbex! I think Probot DCO supporting a "manual" sign-off makes a lot of sense, especially if y'all are already using it. We can likely grab some code from my action (or not if it isn't applicable) as prior art for that feature.

@mmarchini
Copy link
Contributor

I haven't specifically tried it, but (despite forks being generally different) I don't know of any reason this shouldn't work on a fork. It's setup to push to the branch/repo that the PR is based on, so in that case it'd be the fork.

There are two issues: the Action will run on the base repository context (so not on the fork), and the GITHUB_TOKEN (which is used to commit back to the repository) is read-only (which means it can't commit back). So far I was not able to get it working on any repository I tried, and I've seen other projects trying it without success. I would love to see it working though, because it would make our commit queue proposal much easier.

You can read more about the restrictions for pull requests events for forked projects here.

@mmarchini
Copy link
Contributor

Do we have a timeline for this? I think we have the minimal requirements on Actions to implement some automation today: we can use pull_request_target to check if the commit has Signed-off-by statement and leave a comment if it doesn't instructing the user to sign off by commenting /sign-off (or /sign-off User Name <email>), which will then be handled by issue_comment Action which will add a signed-off label. Then we can have ncufind the comment and addSigned-off-by` as part of the commit metadata before landing. I can help implement the Actions and change ncu for this to work if we still want to move forward with it.

@jasnell
Copy link
Member Author

jasnell commented Aug 17, 2020

The last update on this we had was that folks at the Foundation were taking a look at automation necessary for making this seamless. At this point, without having any clear schedule or guideline from the Foundation on that, I think we should proceed with our own approach.

  1. Can the person landing the PR add the Signed-off-by themselves? The answer actually is yes if there is an explicit comment in the PR thread from the original poster indicating that they agree.
  2. What if a PR lands without the Signed-off-by? A comment in the PR thread is sufficient to handle this case.
  3. Can a PR land if there is no sign off? Let's say a PR is ready to land in every other way but the original poster does not respond to requests to add or approve a Signed-off-by... can the PR still land? To be honest, I don't know. We need clarification from legal counsel on this.

@brianwarner
Copy link

Yes, I'm very sorry... between the pandemic, having the kids at home since March 13th, and a bunch of other factors this has slowed down tremendously.

There are a few things you can do here, at least until I can get the last test cases written for the changes to Probot DCO, and final approval from legal on using remediation commits. Probot DCO does allow moderators to override/pass a failing branch. Let me check with legal if an affirmation in the comment stream is sufficient for a DCO signoff. The only difference I can see from the regular DCO process is that it's not in the git log, so it is neither immutable nor portable, but I'll see if I can get an answer for you.

Also, I'll have to ask about the last case. This is a question that would need to be answered by counsel, as it may vary by the circumstances.

@jasnell
Copy link
Member Author

jasnell commented Aug 17, 2020

Thanks @brianwarner ... and no need to apologize at all!

@mmarchini
Copy link
Contributor

mmarchini commented Aug 17, 2020

Probot DCO does allow moderators to override/pass a failing branch

FYI we don't use "failing branch" (if it's the feature I'm thinking of). We'd still need to implement something on ncu...

@mmarchini
Copy link
Contributor

...or streamline our process to rely less on in-house tools and more on GitHub features, which is a whole other discussion :)

@jasnell jasnell closed this Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants