| jbryce | i think we came up with a way that covers our legal bases until June 30 for CLA and then a switch to DCO on july 1. i'll make a comment on the gerrit as well, but really appreciate all the input to help get a plan together on this. | 00:40 |
|---|---|---|
| jbryce | i +1-ed and made a comment. ping me if you think i need to add anything else | 00:44 |
| gmaan | thanks jbryce | 01:45 |
| gouthamr | perfect ty | 01:51 |
| opendevreview | Goutham Pacha Ravi proposed openstack/governance master: Fix outdated info on the tc-guide https://review.opendev.org/c/openstack/governance/+/950446 | 06:13 |
| opendevreview | Goutham Pacha Ravi proposed openstack/governance master: Fix outdated info on the tc-guide https://review.opendev.org/c/openstack/governance/+/950446 | 06:14 |
| opendevreview | Goutham Pacha Ravi proposed openstack/governance master: Fix outdated info on the tc-guide https://review.opendev.org/c/openstack/governance/+/950446 | 06:15 |
| clarkb | looks like the quantum pypi package ownership change is complete fwiw | 15:59 |
| frickler | so according to the sandbox testing, gerrit doesn't require a dco for other's editing or rebasing a change. not sure if that is a bug or whether we'd need an additional test in this case | 16:53 |
| clarkb | frickler: hrm when I tested it did | 16:54 |
| clarkb | though I teseted on a held node and not the sandbox repo | 16:54 |
| clarkb | ok I see your test and can confirm it works. I think the difference is that there is already a signed off by for the commit from the original author | 16:56 |
| clarkb | when I tested the edits the existing change did not have a signed off by | 16:56 |
| frickler | so I guess we need a clear definition on who's signoff is actually needed | 16:58 |
| clarkb | so I guess the code I linke to on the thread is not the same code that processes edits? it definitely seems to check committer and author and "me" in some cases | 16:58 |
| clarkb | fwiw I'm not sure if anyone has ever tested this for the cla enforcement either | 16:59 |
| clarkb | (since the web ui buttons for these thigns came later) | 16:59 |
| clarkb | imo (and I'm not a lawyer) simply rebasing a commit probably does not need a new signed off by. You aren't changing anything about the content of the commit you're just do administrivia so the order is right | 17:00 |
| clarkb | but edits with content changes would ideally have a corresponding signed off by | 17:00 |
| clarkb | (again I'm not a lawyer) | 17:00 |
| JayF | I'll note Gentoo policy is that (effectively) both committer and author need to do signoff; but the flow there is a little different as they do manual merges to master branches. | 17:03 |
| JayF | Honestly, if the natural result is "if you modify it enough that you needed to download/repush" being the case that a separate committeer needs a signoff, that sounds sensible to me ... but obviously I'm not a lawyer :D | 17:04 |
| clarkb | JayF: ya and my read of the code is that if you git push then gerrit is checking committer and author and also that the current account is one of those | 17:04 |
| JayF | but if I edited to fix a minor typo or hit the rebase button it wouldn't? | 17:04 |
| clarkb | its the "click buttons in web ui" case that is different | 17:04 |
| clarkb | and ya maybe click buttons in ui for simple things don't rise to that level | 17:05 |
| JayF | If that's the behavior it makes the maximum sense to jay-the-human but I guess lawyers have to signoff too :D | 17:05 |
| clarkb | but also someone should test the git push case as my read of the code could be wrong | 17:08 |
| clarkb | https://gerrit.googlesource.com/gerrit/+/refs/tags/v3.10.6/java/com/google/gerrit/server/git/validators/CommitValidators.java#710 is the code fwiw | 17:08 |
| clarkb | oh maybe we allow forgecommitter | 17:08 |
| clarkb | forgeAuthor = group Registered Users and forgeCommitter = group Project Bootstrappers are in the All Projects config. so generally no we don't allow forge committer | 17:10 |
| clarkb | and then only stackalytics seems to allow forgeCommitters | 17:10 |
| clarkb | so I don't think that is it | 17:10 |
| clarkb | hrm removing the signed off by completely fails. but having just mine or just gouthamr's works. I'd be curious if someone other than gouthamr or myself tried to adit the current state of https://review.opendev.org/c/opendev/sandbox/+/950608 if it works since then you'd be neither the person who owns the change and the signed off by wouldn't match you | 17:17 |
| fungi | maybe it just checks that there's a signed-off-by footer and not whether it matches the committer nor author? | 17:19 |
| clarkb | fungi: if you push with no signed off by at all you get the rror message at https://gerrit.googlesource.com/gerrit/+/refs/tags/v3.10.6/java/com/google/gerrit/server/git/validators/CommitValidators.java#714 and I can't find anywhere else in the code that uses that error message | 17:19 |
| clarkb | fungi: and if you read that code it looks like it is supposed to check each of them | 17:20 |
| clarkb | but now that I've said this out loud I think I see a bug | 17:20 |
| clarkb | actually no they are doing |= (I thought it was just = and could reset values) | 17:21 |
| clarkb | oh I see it | 17:22 |
| fungi | strangely, my ssh key is being denied by gerrit now... troubleshooting | 17:22 |
| clarkb | https://gerrit.googlesource.com/gerrit/+/refs/tags/v3.10.6/java/com/google/gerrit/server/git/validators/CommitValidators.java#710 that condition requires every single one of the checks to fail for the trigger to fail | 17:23 |
| fungi | never mind, it was ssh running outside the agent context | 17:23 |
| clarkb | I think strict enforcement requires it to be if(!(sboAuthor && sboCommitter && !isMe)) {} | 17:23 |
| fungi | i've confirmed there aren't unexpected accounts left in the Administrators or Project Bootstrappers groups, so that wouldn't explain it | 17:24 |
| clarkb | fungi: ya I think the condition is simply "wrong" | 17:24 |
| fungi | lovely | 17:24 |
| clarkb | now it may be correct for other use cases | 17:24 |
| fungi | probably good enough for now but needs fixing soon | 17:24 |
| clarkb | in the case of gouthamr is author and signed off by and I'm commiter and pusher. We'd get sboAuthor = True, sboCommitter = False, sboMe = False. That then evaluates to !True && !False && !False == False && True && True and skip the block | 17:26 |
| fungi | my non-lawyer attempt to channel the recently deceased spirit of an actual ip lawyer would say that it's fine since this really just fails to catch some possible accidental missing signed-off-by attestations since people are generally amenable to the terms and would have applied them if it weren't for a mistake or error | 17:26 |
| fungi | so it's merely a (rare) bookkeeping error | 17:27 |
| clarkb | the code was last updated by spearce in 2017 | 17:27 |
| clarkb | I'm worried that if I push a change to fix it upstream will balk because we could suddnely break a bunch of workflows | 17:28 |
| JayF | fungi: I have a friend who ended up doing tech in a legal-adjacent job in the US gov for a while, he said that irl lawyers he worked with found it funny when tech folks would get worked up over these tiny "holes" and that the laws didn't really work that way | 17:28 |
| fungi | someone to blame! (just kidding) | 17:28 |
| fungi | JayF: yesh, it's a trope on the debian-legal ml that techies want to treat the law like it's some sort of deterministic computer program | 17:29 |
| clarkb | I'm digging through gerrit history to see if this was intentionally a choice to allow any one of them to be good | 17:31 |
| clarkb | there is a lot of refactoring... | 17:32 |
| clarkb | https://gerrit.googlesource.com/gerrit/+/007c4da18a9ef4e70c5bc57b285b4ba447d473fe we have an answer. It is intentional all the way from 2009 | 17:38 |
| fungi | wow. a 16-year-old decision | 17:39 |
| fungi | older than openstack (barely) | 17:39 |
| clarkb | I think if we want to change this in Gerrit we're going to have to put it behind a config option. Something like strictSignedOfBy = True and then toggle the behavior based on that. That is likely well beyond my "just hack it up" abilities and would require me to understand how to read that from configs and write tests etc (all that to say I'm not sure I've got the capability | 17:39 |
| clarkb | right now) | 17:39 |
| fungi | makes sense | 17:40 |
| clarkb | that said I think this is doable if someone sits down and writes the change and tests it and makes it configurable | 17:41 |
| clarkb | or maybe we'll decide we like the behavior android wants | 17:52 |
| clarkb | its a balance between ease of use and complete control for sure | 17:52 |
| gouthamr | on this specific issue, maybe human reviewers can ask the committer to not delete the Signed-off-by line intentionally, or by accident | 18:18 |
| frickler | I don't like the idea of putting more burden onto reviewers, in my experience such things get easily ignored, like checking for non-voting jobs on relevant changes | 18:46 |
| gouthamr | true; we can't block this waiting on a fix in gerrit though | 18:51 |
| gouthamr | jbryce: got a LF merger and bylaws question for you on https://review.opendev.org/c/openstack/governance/+/949432 | 18:51 |
| gouthamr | ptal when possible, no rush | 18:52 |
| frickler | zuul has a dco-license job, but it looks broken? might be a bit heavy handed, but if it were more reliable, we could go with that? https://zuul.opendev.org/t/openstack/build/6d5317ca4b6f48d19d3dee0293767d4d | 19:09 |
| fungi | i think the dco check job was being used once upon a time by kata on github when there wasn't a great dco enforcement option there | 19:23 |
| opendevreview | Ivan Anfimov proposed openstack/openstack-manuals master: Remove information about SUSE / openSUSE https://review.opendev.org/c/openstack/openstack-manuals/+/950247 | 21:46 |
| clarkb | do we want to draft an email for gerrit's mailing list asking about making the check stricter? Its probably a bit early to do that and we should get more calrification? | 22:06 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!