Automatically add DCO signoff with commit hooks by hausdorff · Pull Request #2283 · envoyproxy/envoy

hausdorff · 2017-12-29T09:33:18Z

Fixes #2281. DCO signoff is mandatory for all contributors. Currently
this is enforced via CI.

This commit smoothes this process somewhat by introducing:

A git commit hook, which will check the commit message for a
signoff, and if not present, add one to the bottom of the message.
A script, support/bootstrap, which will symlink these commit
hooks into the Envoy root .git directory, to minimize the effort
it takes to use them.

Signed-off-by: Alex Clemmer [email protected]

Fixes envoyproxy#2281. DCO signoff is mandatory for all contributors. Currently this is enforced via CI. This commit smoothes this process somewhat by introducing: 1. A git commit hook, which will check the commit message for a signoff, and if not present, add one to the bottom of the message. 2. A script, `support/bootstrap`, which will symlink these commit hooks into the Envoy root `.git` directory, to minimize the effort it takes to use them. Signed-off-by: Alex Clemmer <[email protected]>

mattklein123 · 2017-12-29T17:46:53Z

Sweet! Can we potentially merge https://github.com/envoyproxy/envoy/blob/master/tools/pre-commit.sh into this commit also? That one sets up a hook to check format, but it has never been well documented (I use it locally).

hausdorff · 2017-12-29T18:46:06Z

@mattklein123 Done. Two more things:

Are there any other places we should be documenting this?
I notice that some of the mac OS X tests are failing but because I am not changing any "real" code I'm guessing they're just flaky. So first of all, are they? And second of all is there a way for normals to re-trigger a build in such a case?

hausdorff · 2017-12-29T18:51:17Z

Oh I've noticed that moving the commit hook causes it to not work. Oops. That's what I get for copy+paste'ing. Let me fix that.

hausdorff · 2017-12-29T19:13:24Z

Ok that should be fixed. I went ahead and just rebased the commit because it looks like no one had gotten around to reviewing it.

An undocumented pre-commit hook that runs the format checker on all changed files exists in `tools/pre-commit.sh`. We're transitioning to a model where commit hooks are installed with `support/bootstrap`, so this commit will: 1. Move this file to `support/hooks` 2. Cause `support/bootstrap` to actually install it. Signed-off-by: Alex Clemmer <[email protected]>

mattklein123

This is really awesome. Thank you. Just a few doc comments. I will defer to @htuch on the script review as he is a bash master.

mattklein123 · 2017-12-29T22:51:52Z

support/README.md

+# Support tools
+
+A collection of helper scripts meant to be used by the Envoy developers. For
+example, scripts executed as post-commit hooks would go here.


Would it be worth it to add a little more text here of which things are installed and roughly what they do? I might mention that when the check format script is installed the developer can use the -n flag with git commit to avoid checking if they don't want to temporarily. Some people might get annoyed if they don't know this.

Also, perhaps we should link to here from the top level developer guide? https://github.com/envoyproxy/envoy/blob/master/DEVELOPER.md or even https://github.com/envoyproxy/envoy#contributing?

@mattklein123 Sounds good, let's do all of those things. I'll take another pass later today.

Signed-off-by: Alex Clemmer <[email protected]>

hausdorff · 2017-12-30T01:21:39Z

@mattklein123 ok, I believe all of those comments should be addressed.

htuch

This is awesome. A few design/flow related comments, I think any improvement here is a good direction.

htuch · 2017-12-30T02:22:16Z

support/README.md

+so:
+
+```bash
+git commit --no-verify


Before getting into too much review depth, I'm wondering if we really want this to be opt-out on commit vs. push. My common workflow (and I'm sure is the case for many devs) is that we commit early-and-often locally, without caring about format or DCO (which happens via my git aliases anyway). I later squash down in a rebase and then push. I rather that any additional latency is added on the remote push rather than the local commit.

Of course, these are just my preferences and I don't need to actually use these hooks, but I think they may be shared by others as well.

htuch · 2017-12-30T02:24:28Z

support/bootstrap

+# copying project-standard git commit hooks that do things like ensure DCO
+# signoff is present during commit.
+
+# Best-effort check that we're in the envoy root directory.


Can't you figure out the exact root using git rev-parse --show-toplevel?

I'm not 100% sure that works in all the corner cases, but it looks like it works for submodules, which does seem to be a case we care about, so I'm ok doing this.

htuch · 2017-12-30T02:25:29Z

support/bootstrap

+# Try to find the `.git` directory, even if it's not in Envoy project root (as
+# it wouldn't be if, say, this were in a submodule). The "blessed" but fairly
+# new way to do this is to use `--git-common-dir`.
+dotGitDir=`git rev-parse --git-common-dir`


We tend to prefer $(some subshell) style in Envoy.

htuch · 2017-12-30T02:26:45Z

support/bootstrap

+dotGitDir=`git rev-parse --git-common-dir`
+if test ! -d "${_gitdir}"; then
+  # If `--git-common-dir` is not available, fall back to older way of doing it.
+  dotGitDir=`git rev-parse --git-dir`


Prefer DOT_GIT_DIR style in Envoy scripts.

htuch · 2017-12-30T02:28:54Z

support/bootstrap

+fi
+
+hooksDir="${dotGitDir}/hooks"
+hooksDirRelpath=`relpath "${hooksDir}" "${PWD}"`


Why do we need relative path here? Can't we just symlink from the absolute path?

I'm not an expert in how this works -- my understanding is that if the repostitory is mv'd, the absolute path strategy would fail. Am I wrong?

Either way, in general it just seems nicer to me to have a symlink between two things whose relative path is well-defined, vs. having that knowledge depend on the directory they could sit in.

htuch · 2017-12-30T02:30:46Z

support/hooks/prepare-commit-msg

@@ -0,0 +1,23 @@
+#!/usr/bin/env bash
+#
+# A git commit hook that will automatically append a DCO signoff to the bottom


Could we just enforce that a DCO is present? This would allow flows where the user automatically appends DCO (which I have already) but ensures that there are no mistakes made (trust but verify).

(Handled in the discussion below.)

hausdorff · 2017-12-30T09:37:19Z

@htuch Ok, so, to recap, it sounds like we have two main questions:

When should the formatting checks happen? (Options are: pre-commit, post-commit, and pre-push.)
Should DCO signoff line be added automatically, or should we just error out and tell them that they don't have the signoff line?

For the first, I think you're right that it's more annoying to run as a pre-commit hook, but I suspect that making it a pre-push hook is noticably more complex. (i.e., it's not yet clear to me whether there's an easy way to deal with all the cases of push simply). So what I will offer to you is: if you believe it is worth this extra complexity, both in terms of our time commitment on this PR and in terms of the technical risk, we can do it that way.

For the second, I actually prefer to have it just auto-append if missing, but I'm only about a 6/10 in my conviction this is the way to go, so if you feel strongly about it we can do it your way. My rationale was, firstly, in general I prefer to solve the problem if we can, rather than haranguing people to solve it themselves. But secondly, I wrote this code to work well with other popular workflows I know about: specifically, I wrote it to be compatible with --signoff. Do you see a place where this would be incompatible with another common workflow?

htuch · 2017-12-30T09:52:30Z

Can you elaborate on why it is much more complex to do this on push vs. commit? My understanding (from some quick searching) is that git has pre-push hooks. Can't they just validate some reasonable suffix of commit history (e.g. last 50 commits) have DCO? Anything that comes from upstream should have DCO, any local changes must have had them added.
Since it's compatible with --signoff I think what you have there is good, so no worries.

BTW, I think this whole PR is a really useful contribution and don't want perfect to get in the way of good, so please only do what is low hanging fruit.

hausdorff · 2017-12-30T18:37:17Z

Ah, I was actually talking about the case of running the format checker as pre-push, could be expensive if we validate too many commits. But this case is purely hypothetical at this point, I'm only cautious because we got it wrong on Mesos and it made the push workflow painful. So let me investigate and come back with a real recommendation.

htuch · 2017-12-30T20:48:50Z

The format check doesn't happen on a per-commit basis. It just looks at the entire state of the tree when you run it, without looking at version control history, so I don't think you have a per-commit validation worry there.

Signed-off-by: Alex Clemmer <[email protected]>

htuch

Looks great, thanks for polishing this up.

htuch · 2017-12-30T22:14:56Z

support/hooks/pre-push

+		for i in $(git diff --name-only $RANGE --diff-filter=ACMR 2>&1); do
+			echo "  Checking format for $i"
+			"$SCRIPT_DIR"/check_format.py check $i
+			if [[ $? -ne 0 ]]; then


Can just run set +e, that will catch other badness.

htuch · 2017-12-30T22:15:30Z

support/hooks/pre-push

+do
+	if [ "$LOCAL_SHA" = $DUMMY_SHA ]
+	then
+		# Branch deleted. Do nothing.


Can you use two space indents? That's what we've been doing in general across most languages and shell scripts in the Envoy code base. Thanks.

hausdorff · 2017-12-30T22:17:59Z

@htuch Alright, we're really starting to strain my knowledge of bash, so if it looks like we're doing something dumb, we probably are.

I am pretty sure pre-push won't behave perfectly in all the weird corner cases, but I think we're starting to get close to something that will work for, like, maybe 95% of cases.

htuch

LGTM modulo two space indents.

Signed-off-by: Alex Clemmer <[email protected]>

hausdorff · 2017-12-30T22:24:32Z

@htuch I actually fixed that as you typed that message. I think we're good now.

Signed-off-by: Alex Clemmer <[email protected]>

mattklein123

Sweet, thanks! Will let @htuch merge assuming everything is satisfactory.

htuch

Rad.

Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: JP Simard <[email protected]>

hausdorff force-pushed the dco-hooks branch from 1de0909 to 9d53adc Compare December 29, 2017 19:12

hausdorff force-pushed the dco-hooks branch from 9d53adc to a78e739 Compare December 29, 2017 19:15

mattklein123 reviewed Dec 29, 2017

View reviewed changes

Substantially improve documentation in support/

bd998c6

Signed-off-by: Alex Clemmer <[email protected]>

htuch reviewed Dec 30, 2017

View reviewed changes

Clean up style in support/bootstrap

77f95da

Signed-off-by: Alex Clemmer <[email protected]>

htuch reviewed Dec 30, 2017

View reviewed changes

htuch previously approved these changes Dec 30, 2017

View reviewed changes

hausdorff added 2 commits December 30, 2017 15:19

Move format check from pre-commit to pre-push

0294a07

Signed-off-by: Alex Clemmer <[email protected]>

Use git to detect whether we're in the root

a6209c1

Signed-off-by: Alex Clemmer <[email protected]>

hausdorff dismissed htuch’s stale review via a6209c1 December 30, 2017 22:19

hausdorff force-pushed the dco-hooks branch from 26eae40 to a6209c1 Compare December 30, 2017 22:19

More docs fixes

da7a2a8

Signed-off-by: Alex Clemmer <[email protected]>

mattklein123 approved these changes Dec 31, 2017

View reviewed changes

htuch approved these changes Dec 31, 2017

View reviewed changes

htuch merged commit a38facb into envoyproxy:master Dec 31, 2017

isaachier mentioned this pull request Jun 12, 2018

Why sign each and every commit? jaegertracing/jaeger#866

Closed

wyli mentioned this pull request May 19, 2022

4293 local pre-commit check support Project-MONAI/MONAI#4292

Merged

7 tasks

jpsim pushed a commit that referenced this pull request Nov 28, 2022

test: adding a cancel test, cleaning up copy-paste code (#2283)

f194207

Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: JP Simard <[email protected]>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

test: adding a cancel test, cleaning up copy-paste code (#2283)

b5afc6a

Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: JP Simard <[email protected]>

Conversation

hausdorff commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Dec 29, 2017

Uh oh!

hausdorff commented Dec 29, 2017

Uh oh!

hausdorff commented Dec 29, 2017

Uh oh!

hausdorff commented Dec 29, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hausdorff commented Dec 30, 2017

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hausdorff commented Dec 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Dec 30, 2017

Uh oh!

hausdorff commented Dec 30, 2017

Uh oh!

htuch commented Dec 30, 2017

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hausdorff commented Dec 30, 2017

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

hausdorff commented Dec 30, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

hausdorff commented Dec 29, 2017 •

edited

Loading

hausdorff commented Dec 30, 2017 •

edited

Loading