[libvirt] [RFC] Adopting 'Tested-by' tag (and probably other tags)?
Hi folks, I was wondering if the upstream would be receptive to adding a 'Tested-by' tag for patches that had someone give tested feedback on the list. Personally, what I consider a bar for giving a 'Tested-by' is, when someone: - Applies a patch or a series (following the iterations as needed, over time) locally, compile them, spend time understanding the functional change in question, and its implications.) - Do a concrete test (w/ either a Python API or a `virsh`-based command-line) that exercises the said code path. - *Post* the above test procedure / and necessary outputs as evidence to the list, in reasonable detail. (Not a: "yeah, I tested and it works" -- this isn't a 'rule'; it can have exceptions)
From a quick glance, in its 10-year history, libvirt upstream had about a mere 21 entries of 'Tested-by' tags:
$ git log | grep Tested-by | wc -l 21 I bring this up because, when someone spends effort following (sometimes over weeks / months) a patch (or a series) from the list, gives reasonably detailed test feedback, in my books, it would be fair to acknowledge it in the Git. And it will encourage them to spend time in future. --- I realize that if it's not automated (via Git hooks or similar), it can become "lossy", i.e. if Joe posts v1 of a patch, you give a 'Tested-by', then there are two scenarios that immediately spring to mind: (1) Joe respins a v2 to make some corrections, adds your 'Tested-by' tag, and whoever applies the patch picks it up -- all good. (b) However, if a v2 was _not_ necessary, then whoever is applying the patch / series must remember to add the tag -- "lossy". Thoughts / remarks / rotten tomatoes welcome. -- /kashyap
On 04/26/2017 09:55 AM, Kashyap Chamarthy wrote:
Hi folks,
I was wondering if the upstream would be receptive to adding a 'Tested-by' tag for patches that had someone give tested feedback on the list.
If we are doing this, I suggest having 'Reviewed-by' too. It's merely the same effort (if not a greater one).
I realize that if it's not automated (via Git hooks or similar), it can become "lossy", i.e. if Joe posts v1 of a patch, you give a 'Tested-by', then there are two scenarios that immediately spring to mind:
(1) Joe respins a v2 to make some corrections, adds your 'Tested-by' tag, and whoever applies the patch picks it up -- all good.
(b) However, if a v2 was _not_ necessary, then whoever is applying the patch / series must remember to add the tag -- "lossy".
Thoughts / remarks / rotten tomatoes welcome.
I think this is the clue for having this. If we can somehow make it easy for contributors to append appropriate lines to the commit messages of theirs then this has a chance to live. Otherwise, if I have to manually copy-paste (or even worse write all the lines out manually), then the chances as slim. Take a look at SoB line: it's very easy to configure and everybody uses it now. I like the idea and I'm up for it, but frankly I have no idea how developers in other teams do it. Michal
On Wed, Apr 26, 2017 at 10:27:00 +0200, Michal Privoznik wrote:
On 04/26/2017 09:55 AM, Kashyap Chamarthy wrote:
Hi folks,
I was wondering if the upstream would be receptive to adding a 'Tested-by' tag for patches that had someone give tested feedback on the list.
If we are doing this, I suggest having 'Reviewed-by' too. It's merely the same effort (if not a greater one).
Definitely greater. We don't get that much testing of patches upstream.
I realize that if it's not automated (via Git hooks or similar), it can become "lossy", i.e. if Joe posts v1 of a patch, you give a 'Tested-by', then there are two scenarios that immediately spring to mind:
(1) Joe respins a v2 to make some corrections, adds your 'Tested-by' tag, and whoever applies the patch picks it up -- all good.
(b) However, if a v2 was _not_ necessary, then whoever is applying the patch / series must remember to add the tag -- "lossy".
Thoughts / remarks / rotten tomatoes welcome.
I think this is the clue for having this. If we can somehow make it easy for contributors to append appropriate lines to the commit messages of theirs then this has a chance to live. Otherwise, if I have to manually copy-paste (or even worse write all the lines out manually), then the
I totally agree. If it's somehow automatable in a reasonable fashion to apply the tags from e-mail to git then I might consider it.
chances as slim. Take a look at SoB line: it's very easy to configure and everybody uses it now.
I beg to differ.
I like the idea and I'm up for it, but frankly I have no idea how developers in other teams do it.
Unless there's reasonable automation, which I don't think will be easy I will tend to not put those in. I'm lazy.
On Wed, Apr 26, 2017 at 09:55:50AM +0200, Kashyap Chamarthy wrote:
Hi folks,
I was wondering if the upstream would be receptive to adding a 'Tested-by' tag for patches that had someone give tested feedback on the list.
Personally, what I consider a bar for giving a 'Tested-by' is, when someone:
- Applies a patch or a series (following the iterations as needed, over time) locally, compile them, spend time understanding the functional change in question, and its implications.)
- Do a concrete test (w/ either a Python API or a `virsh`-based command-line) that exercises the said code path.
- *Post* the above test procedure / and necessary outputs as evidence to the list, in reasonable detail. (Not a: "yeah, I tested and it works" -- this isn't a 'rule'; it can have exceptions)
From a quick glance, in its 10-year history, libvirt upstream had about a mere 21 entries of 'Tested-by' tags:
$ git log | grep Tested-by | wc -l 21
I bring this up because, when someone spends effort following (sometimes over weeks / months) a patch (or a series) from the list, gives reasonably detailed test feedback, in my books, it would be fair to acknowledge it in the Git. And it will encourage them to spend time in future.
---
I realize that if it's not automated (via Git hooks or similar), it can become "lossy", i.e. if Joe posts v1 of a patch, you give a 'Tested-by', then there are two scenarios that immediately spring to mind:
(1) Joe respins a v2 to make some corrections, adds your 'Tested-by' tag, and whoever applies the patch picks it up -- all good.
(b) However, if a v2 was _not_ necessary, then whoever is applying the patch / series must remember to add the tag -- "lossy".
I don't think that's a big deal really, QEMU has been doing this for years. If you post a vNNN of your patch, you are responsible for adding the tags. When the sub-tree maintainer accepts your patch they add any outstanding tags, as well as their own S-o-B. This is little work compard to actually applying & testing the patch before pushing it
Thoughts / remarks / rotten tomatoes welcome.
I'd like to see us formally adopt the signed-off-by approach for all patches as a mandatory thing, along with the associated contributor convenant. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Apr 26, 2017 at 09:35:47AM +0100, Daniel P. Berrange wrote:
On Wed, Apr 26, 2017 at 09:55:50AM +0200, Kashyap Chamarthy wrote:
[...]
I realize that if it's not automated (via Git hooks or similar), it can become "lossy", i.e. if Joe posts v1 of a patch, you give a 'Tested-by', then there are two scenarios that immediately spring to mind:
(1) Joe respins a v2 to make some corrections, adds your 'Tested-by' tag, and whoever applies the patch picks it up -- all good.
(b) However, if a v2 was _not_ necessary, then whoever is applying the patch / series must remember to add the tag -- "lossy".
I don't think that's a big deal really, QEMU has been doing this for years. If you post a vNNN of your patch, you are responsible for adding the tags.
Nod; funnily enough, Markus Armbruster sort of mentioned this 'lossy' notion with QEMU sometimes, with a note that: seasoned contributors / maintainers are all fairly diligent, but sometimes mistakes happen.
When the sub-tree maintainer accepts your patch they add any outstanding tags, as well as their own S-o-B. This is little work compard to actually applying & testing the patch before pushing it
Noted. One of the reasons for sending this email was also my experience interacting with upstream QEMU community, where the said tags are indeed more rigorously used.
Thoughts / remarks / rotten tomatoes welcome.
I'd like to see us formally adopt the signed-off-by approach for all patches as a mandatory thing, along with the associated contributor convenant.
By "contributor covenant", I presume you're referring to: http://contributor-covenant.org/ http://contributor-covenant.org/version/1/4/code_of_conduct.txt -- /kashyap
On Wed, Apr 26, 2017 at 11:06:54AM +0200, Kashyap Chamarthy wrote:
On Wed, Apr 26, 2017 at 09:35:47AM +0100, Daniel P. Berrange wrote:
On Wed, Apr 26, 2017 at 09:55:50AM +0200, Kashyap Chamarthy wrote:
[...]
I realize that if it's not automated (via Git hooks or similar), it can become "lossy", i.e. if Joe posts v1 of a patch, you give a 'Tested-by', then there are two scenarios that immediately spring to mind:
(1) Joe respins a v2 to make some corrections, adds your 'Tested-by' tag, and whoever applies the patch picks it up -- all good.
(b) However, if a v2 was _not_ necessary, then whoever is applying the patch / series must remember to add the tag -- "lossy".
I don't think that's a big deal really, QEMU has been doing this for years. If you post a vNNN of your patch, you are responsible for adding the tags.
Nod; funnily enough, Markus Armbruster sort of mentioned this 'lossy' notion with QEMU sometimes, with a note that: seasoned contributors / maintainers are all fairly diligent, but sometimes mistakes happen.
When the sub-tree maintainer accepts your patch they add any outstanding tags, as well as their own S-o-B. This is little work compard to actually applying & testing the patch before pushing it
Noted. One of the reasons for sending this email was also my experience interacting with upstream QEMU community, where the said tags are indeed more rigorously used.
Thoughts / remarks / rotten tomatoes welcome.
I'd like to see us formally adopt the signed-off-by approach for all patches as a mandatory thing, along with the associated contributor convenant.
By "contributor covenant", I presume you're referring to:
http://contributor-covenant.org/ http://contributor-covenant.org/version/1/4/code_of_conduct.txt
No, I mixed up the terms. What I actually meant was the Developer Certificate of Origin https://developercertificate.org/ The use of a 'Signed-off-by' indicates acceptance of this DCO (or similar) in projects which formally adopt S-o-B usage. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Apr 26, 2017 at 10:10:08AM +0100, Daniel P. Berrange wrote:
On Wed, Apr 26, 2017 at 11:06:54AM +0200, Kashyap Chamarthy wrote:
[...]
I'd like to see us formally adopt the signed-off-by approach for all patches as a mandatory thing, along with the associated contributor convenant.
By "contributor covenant", I presume you're referring to:
http://contributor-covenant.org/ http://contributor-covenant.org/version/1/4/code_of_conduct.txt
No, I mixed up the terms. What I actually meant was the Developer Certificate of Origin
https://developercertificate.org/
The use of a 'Signed-off-by' indicates acceptance of this DCO (or similar) in projects which formally adopt S-o-B usage.
Ah-ha, makes sense. Now I recall that the initiator of this is the Linux Kernel: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-... [...] -- /kashyap
On 04/26/2017 05:10 AM, Daniel P. Berrange wrote:
On Wed, Apr 26, 2017 at 11:06:54AM +0200, Kashyap Chamarthy wrote:
On Wed, Apr 26, 2017 at 09:35:47AM +0100, Daniel P. Berrange wrote:
On Wed, Apr 26, 2017 at 09:55:50AM +0200, Kashyap Chamarthy wrote:
[...]
I realize that if it's not automated (via Git hooks or similar), it can become "lossy", i.e. if Joe posts v1 of a patch, you give a 'Tested-by', then there are two scenarios that immediately spring to mind:
(1) Joe respins a v2 to make some corrections, adds your 'Tested-by' tag, and whoever applies the patch picks it up -- all good.
(b) However, if a v2 was _not_ necessary, then whoever is applying the patch / series must remember to add the tag -- "lossy".
I don't think that's a big deal really, QEMU has been doing this for years. If you post a vNNN of your patch, you are responsible for adding the tags.
Nod; funnily enough, Markus Armbruster sort of mentioned this 'lossy' notion with QEMU sometimes, with a note that: seasoned contributors / maintainers are all fairly diligent, but sometimes mistakes happen.
When the sub-tree maintainer accepts your patch they add any outstanding tags, as well as their own S-o-B. This is little work compard to actually applying & testing the patch before pushing it
Noted. One of the reasons for sending this email was also my experience interacting with upstream QEMU community, where the said tags are indeed more rigorously used.
Thoughts / remarks / rotten tomatoes welcome.
So not opposed to the concept - although like anything it could take some period of "getting used to using it". I can certainly see it being forgotten more frequently as we reach the end of each month and everyone pushes to get changes in... Since the libvirt community is far smaller, I think we'll rarely see T-b other than if the testing is by the S-o-B. I can recall only reading a few patch reviews over the last couple of years where someone noted that they also tested the patch [series]. Although it does happen and should then be attributed. Still for the most part, I would "hope" that an S-o-B could come with the implicit expectation that they've run "make check syntax-check". Similarly a reviewer would I think for a majority of what they review, apply the patches and at least build them. Many times when I have patches that I'm not sure will trip up Coverity - I'll run them through a test coverity build too, but I don't attribute that ever as that seems to draw unwanted attention sometimes... As for Reviewed-by - I do see that being something that's a bit more useful. Although how does one attribute R-b for trivial or build breaker patches? Since they aren't "automagically added", that means adding them requires the Submittor to actually make the effort. So if that's then the case that every submit has to have them, how does that get enforced? With QEMU I understand that's a bit easier with owners of sub-tree's that can enforce that. But libvirt has multiple push capable maintainers, so now each of them becomes responsible for that. What happens if one forgets or one consistently doesn't provide the tags? Is their push privilege taken away? IOW, what's the penalty for not following the accepted community rule? Again, I'm not against this, but sometimes getting *any* commit message for a patch is a struggle for some! Adding tags could be torturous ;-)
I'd like to see us formally adopt the signed-off-by approach for all patches as a mandatory thing, along with the associated contributor convenant.
By "contributor covenant", I presume you're referring to:
http://contributor-covenant.org/ http://contributor-covenant.org/version/1/4/code_of_conduct.txt
No, I mixed up the terms. What I actually meant was the Developer Certificate of Origin
https://developercertificate.org/
The use of a 'Signed-off-by' indicates acceptance of this DCO (or similar) in projects which formally adopt S-o-B usage.
Coming from someone who only more recently in his career has been an open source contributor - I didn't realize the S-o-B usage is related to knowing the existence of said document ;-)... It's not linked from our hacking document either AFAICT. John
On Wed, Apr 26, 2017 at 08:14:31AM -0400, John Ferlan wrote:
Still for the most part, I would "hope" that an S-o-B could come with the implicit expectation that they've run "make check syntax-check". Similarly a reviewer would I think for a majority of what they review, apply the patches and at least build them. Many times when I have patches that I'm not sure will trip up Coverity - I'll run them through a test coverity build too, but I don't attribute that ever as that seems to draw unwanted attention sometimes...
In general S-o-B is /only/ about stating your compliance with the legal side of contribution rules. ie confirming you have the right to submit the patch. Whether they've complied with coding guidelines is tangential.
As for Reviewed-by - I do see that being something that's a bit more useful. Although how does one attribute R-b for trivial or build breaker patches?
S-o-B is considered to be a superset of Reviewed-by. The person who merges the patch to git master would add their own S-o-B line to indicate that they have the right to merge to it, and implicitly that they've reviewed it. IOW, you wouldn't have S-o-B and R-b for the same person on the same patch - just S-o-B is sufficient. This makes it trivial for the person merging the patch to git, since they simply can do git am <the patch> and then "git commit --amend -s" to add their S-o-B
Since they aren't "automagically added", that means adding them requires the Submittor to actually make the effort. So if that's then the case that every submit has to have them, how does that get enforced?
It would be possible to write a git hook that refused a patch unless the patch has a S-o-B from the person doing the push. Likewise the git hook can validate that the patch author has also proivded an S-o-B.
What happens if one forgets or one consistently doesn't provide the tags? Is their push privilege taken away? IOW, what's the penalty for not following the accepted community rule?
Again, I'm not against this, but sometimes getting *any* commit message for a patch is a struggle for some! Adding tags could be torturous ;-)
Dealing with S-o-B is trivial, since it just means adding -s flag to git. The other tags are more manual, but not that much work. The reviewed-by tags serve two purposes - one they give an indication to the subsystem maintainer that the code has a certain level of review and thus might be ready for merging, and two they give a historic record of who did what. In our model with much broader set of committers, I don't think the reviewed-by gives much benefit, so it just becomes a record of who did what - which is already something present in the mailing list. Personally I'd just keep things simple and only require a S-o-B from the patch author, and the person committing. If people want to add other tags fine, but I certaily wouldn't enforce anything other than S-o-B Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Apr 26, 2017 at 01:40:33PM +0100, Daniel P. Berrange wrote:
On Wed, Apr 26, 2017 at 08:14:31AM -0400, John Ferlan wrote:
[...] # Snip some useful discussion
What happens if one forgets or one consistently doesn't provide the tags? Is their push privilege taken away? IOW, what's the penalty for not following the accepted community rule?
Again, I'm not against this, but sometimes getting *any* commit message for a patch is a struggle for some! Adding tags could be torturous ;-)
Dealing with S-o-B is trivial, since it just means adding -s flag to git. The other tags are more manual, but not that much work.
The reviewed-by tags serve two purposes - one they give an indication to the subsystem maintainer that the code has a certain level of review and thus might be ready for merging, and two they give a historic record of who did what. In our model with much broader set of committers, I don't think the reviewed-by gives much benefit, so it just becomes a record of who did what - which is already something present in the mailing list.
Personally I'd just keep things simple and only require a S-o-B from the patch author, and the person committing. If people want to add other tags fine, but I certaily wouldn't enforce anything other than S-o-B
I concur with your view. (I didn't imply that the specific tag mentioned in my email Subject ought to be 'enforced'.) --- Is there any hurdle from adopting the S-o-b approach that you outlined above? Or is it just that it involves getting a consensus-based agreement among upstream contributors, and making the trivial adjustment to the Git workflow like you indicated above, adding: '--signoff' flag to `git commit`? (I guess it's the latter.) Thanks. -- /kashyap
participants (5)
-
Daniel P. Berrange -
John Ferlan -
Kashyap Chamarthy -
Michal Privoznik -
Peter Krempa