Make CI enforce correct license banner + correct DCO sign-off in commit message#9
Make CI enforce correct license banner + correct DCO sign-off in commit message#9martijnthe wants to merge 1 commit intomasterfrom
Conversation
|
Failing the build when the commit message misses the DCO signed-off message can be seen here: |
…it message JerryScript-DCO-1.0-Signed-off-by: Martijn The martijn.the@intel.com
martijnthe
force-pushed
the
check-signed-off-and-license
branch
from
676802f to
8e6c27d
Compare
|
@akosthekiss or perhaps @yichoi could you review? |
| @@ -1,2 +1,3 @@ | |||
| node_modules | |||
| yarn-error.log | |||
| .idea | |||
There was a problem hiding this comment.
If this is just something you do on your system, might be more appropriate to do something like I do:
In /home/pepsi/.gitconfig:
[core]
excludesfile = /home/pepsi/.gitignore_global
In /home/pepsi/.gitignore_global:
*~
\#*
.#*
NOTES.*
There was a problem hiding this comment.
The main project's .gitignore has a bunch of IDE related lines:
https://github.com/jerryscript-project/jerryscript/blob/master/.gitignore#L9
I don't feel strongly about it though, although I've got bitten by system-level gitignore in the past where a repo actually did have files that were being ignored by my own system-level gitignore (and touching those files didn't appear as changed). So I've lived with an empty one ever since...
There was a problem hiding this comment.
Fine with me; I'd never seen .idea before. :)
grgustaf
left a comment
grgustaf
left a comment
There was a problem hiding this comment.
Other than the .idea in .gitignore, LGTM.
| '.py', | ||
| '.S', | ||
| '.sh', | ||
| '.tcl', |
There was a problem hiding this comment.
I guess it makes sense to keep these around to catch future additions?
There was a problem hiding this comment.
Yeah, I considered it. But most of them only make sense in the context of a C project. I think it makes more sense to just add as needed.
There was a problem hiding this comment.
I'm also voting for the shorter list. For now, something like the following might be needed: .sh, .py, .js, .ts?
| @@ -1,2 +1,3 @@ | |||
| node_modules | |||
| yarn-error.log | |||
| .idea | |||
| - stage: lint | ||
| script: yarn lint | ||
| script: | ||
| - ./tools/check-signed-off.sh |
There was a problem hiding this comment.
I think you'll need ./tools/check-signed-off.sh --travis here. When merging a PR, Travis tends to rewrite the author info of the commit with the info found in the public profile of the PR's author. Which can be different from the settings found on the local machine of the author. At least that was the case some time back and caused all validations to check out perfectly while Travis was running for the PR but going red as soon as patches got merged into master. The solution (workaround?) was to be strict when checking the PR but be tolerant when the PR gets merged. (That works if master can progress only through PRs.)
| '.py', | ||
| '.S', | ||
| '.sh', | ||
| '.tcl', |
There was a problem hiding this comment.
I'm also voting for the shorter list. For now, something like the following might be needed: .sh, .py, .js, .ts?
3 participants
Addresses #5
I lifted the checking scripts from the main jerryscript repo and changed the list of directories in the license check script to only contain
srcandtoolsand no excludes.