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 a GitHub Action that auto-checks the existence of Signed-off-by #5

Open
benmont opened this issue Aug 27, 2020 · 10 comments
Open

Comments

@benmont
Copy link
Contributor

benmont commented Aug 27, 2020

On top of that, we can recommend a GitHub Action that auto-checks the existence of the Signed-off-by line. For the open source projects, Actions are free anyway so we maybe even add the Action to the template.

I like the idea of adding the Action to the template.

Originally posted by @benmont in #4 (comment)

@benmont benmont changed the title > On top of that, we can recommend a GitHub Action that auto-checks the existence of the Signed-off-by line. For the open source projects, Actions are free anyway so we maybe even add the Action to the template. Add a GitHub Action that auto-checks the existence of Signed-off-by Aug 27, 2020
@benmont
Copy link
Contributor Author

benmont commented Aug 27, 2020

@mbr0wn FYI I made an Issue for this

@alejandro5042
Copy link
Member

Here's one popular GitHub Action that does this enforcement: https://github.com/probot/dco

[probot/dco] requires all commit messages to contain the Signed-off-by line with an email address that matches the commit author.

However, it does not appear that GitHub supports signing commits from the web UI (todogroup/gh-issues#50), which would make any quick web contributions impossible. Worse, if you forget to sign your commit locally (e.g. committing in VS or VS Code doesn't put this option in the forefront), you'll need to interactive rebase all your commits while signing at each step... also annoying :)

Aside: Here's the developer workflow for signing commits in VS Code

You can auto-signoff everything via: microsoft/vscode#83096

Or you can commit with the signoff option:

image

None of these are intuitive... you'd need to know there was a config token you could set for your workspace to take advantage of it. In order to persist this config token, you'd need to save the workspace.yml file to disk, which you may not want to do and have to remember not to clean out. The signoff option isn't any better: it's hidden in a menu and requires two clicks to use; the checkmark or the even more convenient and default Ctrl+Enter for submitting after typing a message doesn't sign your commit.

Could we do it another way?

@mbr0wn
Copy link
Collaborator

mbr0wn commented Sep 14, 2020

Adding a checkbox to the PR template that you must check

This is exactly what I changed in #4. I would suggest not doing that (hence my PR), as it is not tied to the repo.

@dkapulkin
Copy link

@benmont , could I get some background on why this signed-off-by is recommended (required?) by our NI template? Is it okay to not be using that format in repos that otherwise follow this template? It seems like it's not super common, and, for example, the data-record-ad-internal repo hasn't been enforcing it. We're not making large changes to Linux kernels, or even application software, rather writing plugins. Is the author field not sufficient traceability?

@rdecarreau had the same question :)

@benmont
Copy link
Contributor Author

benmont commented Feb 7, 2022

@mbr0wn ^

@mbr0wn
Copy link
Collaborator

mbr0wn commented Feb 8, 2022

@dkapulkin @rdecarreau et al: When we create a new OSS project, we have to decide how we want to handle contributions from a legal perspective. There's basically three options:

  • Don't care
  • DCO
  • CLA

The first option ("don't care") is something that some smaller open source projects do, but is obviously no option for us. That leaves DCO or CLA.

If you go the DCO route, then you need the Signed-off-by line, because that's how the signing of the DCO is enforced. It is independent of the repo tool we use (i.e., Github), low-tech, simple, and very standard (see Linux kernel, u-boot, GNU Radio, anything from the yocto world, ...).

You can decide to go the CLA route instead, which makes a lot of sense, e.g., if we want to retain copyright on all contributions (DCO does not give you that). However, that means extra work when setting up the repository (as in, working with legal to draft the actual CLA document, defining the contribution requirements, etc.). If you do that extra work, then you don't need the DCO because contributors need to sign a separate document. There's at least one repo at NI that I know of that does that (https://github.com/EttusResearch/uhd/, see, e.g., EttusResearch/uhd#554). In the past, contributors had to download, print, sign, scan, and return to us a PDF document. Now, we use a bot do the signing). However, even if you go this route, there's no harm in keeping the signed-off-by requirement. It acts as an extra reminder that people had to sign something.

CLAs are summarized here:

https://nio365.sharepoint.com/sites/SoftwareDiscipline/SitePages/Open-Source-Tactics.aspx

I assume you've seen the NI Open Source License Compliance Training: https://nio365.sharepoint.com/:p:/r/sites/OpenSource824/_layouts/15/Doc.aspx?sourcedoc=%7BBAC426C5-79E8-4EC2-B15F-B7AF1A5FA85E%7D&file=NI%20Open%20Source%20Licensing%20Training.pptx&action=edit&mobileredirect=true&cid=50e41383-0042-49db-9c77-0b856ebad9d7

@dkapulkin
Copy link

@mbr0wn , thank you very much! I had not done that entire training yet, it was very useful. I'll check whether our Joint Developer Agreement with the external collaborators contained a CLA. I know right now our LICENSE file contains the default DCO.

Should our developers be signing every commit or is it sufficient to sign just the squash commit of a PR going into main? In data-record-ad-internal we follow a branch and merge flow instead of forking so any given branch in the repo will have many commits.

@mbr0wn
Copy link
Collaborator

mbr0wn commented Feb 9, 2022

@dkapulkin Technically, you don't need to sign anything, because as NI employees you've implicitly signed off. NI owns your IP (unless you develop it out of hours, on your own gear, and it's unrelated to your work at NI, etc.). So you can choose a workflow that works best for you.

I recommend that at least all commits on main get signed-off, just for consistency's sake. Outside collaborators might get rubbed the wrong way if they have to sign everything, and internal devs don't. Besides, adding -s to git commit is not a big deal. So if it works better for you, then you can just sign-off the squash commits. I recommend that you add Signed-off-by lines for all authors of the squash commit (the same way you'd likely use Co-authored-by).

@mbr0wn
Copy link
Collaborator

mbr0wn commented Feb 9, 2022

because as NI employees you own squat

Just wanted to point out this was a bad choice of words and I've edited my comment above. I meant to emphasize that you can't accidentally get this wrong as an NI employee (when contributing to an NI-open-source-repo), because you don't own the IP in the first place.

@dkapulkin
Copy link

I understood the intent, no worries :D Our collaborators on our specific repo are operating as contractors under Joint Developer Agreement, so it sounds like they're already subject to the same terms (I can confirm with the team that drew up all the contracts). In either case it doesn't sound like we've broken any rules yet, and I now know what to do when we transition some IP to our public-facing repo. Thanks again for all the help, and sorry if the discussion on this PR wasn't the most appropriate place for this discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants