src-d / modelforge

Python library to share machine learning models easily and reliably.
Apache License 2.0
18 stars 13 forks source link

Add DCO #36

Closed r0mainK closed 6 years ago

r0mainK commented 6 years ago

Reacting to this: https://github.com/src-d/models/pull/4 I've added DCO to the commit message. You can pass it as arguments with committer-name and committer-email flags, or simply define them in modelforgecfg.py or ENV variables like for other "annoying" parameters.

r0mainK commented 6 years ago

@vmarkovtsev this is ready for review/merge. In the end I also added small changes in the index.py file, to manage the case where the pulled git repo does not have an index file. At first I wanted to just put it as a warning, since we could assume the repo does not contain the appropriate file because it's being initialized, but then thought it might be best for the user to initially commit and empty JSON file (containing just {}) to the repo he will use as and index. If you think we should change I can simply add a boolean parameter to the __init__ of GitIndex to catch the error if we're running the init command and fail otherwise

vmarkovtsev commented 6 years ago

@r0mainK So these two global vars are not needed - we should parse the git config file every time we commit.

Also it's better to extract the index changes into a separate PR.

r0mainK commented 6 years ago

@vmarkovtsev gonna check if dulwich implements that, for the time being I extracted the other changes : https://github.com/src-d/modelforge/pull/37

vmarkovtsev commented 6 years ago

To be merged once Travis is happy.

smola commented 6 years ago

Does this mean that users will be signing-off commits unknowingly? This may render sign-offs legally void unless the user is informed before the fact.

r0mainK commented 6 years ago

did not know that ^^" @smola would adding a disclaimer in the README.md and a --no-dco flag circumvent this ?

smola commented 6 years ago

I would say that we need some kind of explicit action by the user such as adding a flag, accepting an interactive prompt, setting a configuration file or something like that, but I'm not really sure.

@eiso Can we get some advice here?

vmarkovtsev commented 6 years ago

@smola good point, I didn't know either

vmarkovtsev commented 6 years ago

I guess the --signoff option must be added to make the DCO optional.

vmarkovtsev commented 6 years ago

Also the configuration flag ALWAYS_SIGNOFF which is False by default.

r0mainK commented 6 years ago

okay making the change, will ping you once it is added

smola commented 6 years ago

@vmarkovtsev IANAL but adding an explicit environment variable / CLI flag that is disabled by default looks on the safe side.

r0mainK commented 6 years ago

@vmarkovtsev added the feature

r0mainK commented 6 years ago

@vmarkovtsev solved the conflicts and did the changes you asked. I also modified the README.md and doc/cmd.md