Check a PR has been committed using git signoff

[NvTimLiu]

1. Pre-merge build fails without seeing a -s signoff from anyone committing to a PR.

   If the author of a commit adds a single commit with a -s, that will be sufficient for the pre-merge build to pass.

2, Only update Jenkins scripts, no source change. No unit tests needed.

3, No Github issue related to the PR.

[NvTimLiu]

build

[jlowe]

We may need to think a bit more about how to handle signoff checking and multiple commits. Looks like the current algorithm short-circuits to true if any commit has a signoff. However there are situations where this can lead to a false-positive (e.g.: branch contains merged commits from another branch that has at least one signoff but other, non-merged commits do not).

[revans2]

@NvTimLiu what are the plans for this? I'd like to see it go in, and I am happy to fix the spelling issue later if you don't have time right now to do it.

[revans2]

build

[jlowe]

@revans2 are we going with only one signoff commit required per PR? Seems this logic would greenlight a PR with 20 commits where only 1 commit has the signoff.

[revans2]

@jlowe I am fine with that, and we can change it in the future if we need to.

[tgravescs]

"Any contribution which contains commits that are not signed off will not be accepted."

https://github.com/NVIDIA/spark-rapids/blob/branch-0.2/CONTRIBUTING.md

[revans2]

So I cannot upmerge without signing off on the merge commit? What if the github merge commit did not have a sign-off on it? Think about the case of merging a commit from one branch to another. I understand what the document says. We wrote that document.

[jlowe]

What if the github merge commit did not have a sign-off on it?

Shouldn't be too long before all the commits pulled in by a merge will have signoffs, but it could be a problem in the short term.

Agree this is better than what we have today which is no checks. Let's go with this and file a followup to either tighten the checks or update the docs.

[tgravescs]

I'm definitely fine with that as like you said its better the no checks

[sameerz]

build

[revans2]

build

[jlowe]

Appears the CI build failed because the last commit doesn't have a signoff:

08:05:28  + git log e129f1b43f5a1f1861e10701214ef496506fe58f..a0f557a51c2f80da8e3c42dd424a0d2ae89446cf --pretty=format:%h
08:05:28  [Pipeline] sh
08:05:29  + git show a0f557a --shortstat
08:05:29  + grep Signed-off-by
[...]
08:05:29  [Pipeline] End of Pipeline
08:05:29  Setting status of a0f557a51c2f80da8e3c42dd424a0d2ae89446cf to FAILURE with url https://ci.ngcc.nvidia.com/job/spark/job/rapids_premerge-github/315/ and message: 'Build finished. '

I'll dig a bit into why it didn't see the first commit in this PR which does have a signoff.

[jlowe]

I think there's an issue here where if grep fails to find a match it will return a non-zero exit code. That in turn will cause the pipeline command to return a non-zero exit code which will cause the sh command to throw an exception as described here. I think this either needs to use the returnStatus: true option and switch off the result code of the step or do something like this to avoid grep failing to match to cause a pipeline failure:

Suggested change

	script: "git show ${commit} --shortstat | grep 'Signed-off-by'")
	script: "git show ${commit} --shortstat | grep 'Signed-off-by || echo'")

[NvTimLiu]

build

[NvTimLiu]

build

[NvTimLiu]

Sorry for the delayed response, I was off for personal issue last week.

Fixed the sh exception when grep not find a match of Signed-off-by in the commit lists of a PR.

@jlowe @revans2 Do you mean we need to make sure the "Signed-off-by" tag exists in every commit?

I've learned from Sameer @sameerz that we can PASS the pre-merge build only if one of the PR commits(instead of all commits) have a Signed-off-by tag.