2:55 a.m.
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
3:27 a.m.
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
3:35 a.m.
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.
3:35 a.m.
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 :|
4:06 a.m.
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
4:10 a.m.
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 :|
4:38 a.m.
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#si...
[...]
--
/kashyap
7:14 a.m.
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
7:40 a.m.
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 :|
10:06 a.m.
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
2767
days inactive
2767
days old
9 comments
5 participants
participants (5)
-
Daniel P. Berrange -
John Ferlan -
Kashyap Chamarthy -
Michal Privoznik -
Peter Krempa