[dpdk-dev] Understanding of Acked-By
thomas.monjalon at 6wind.com
Wed Jan 25 15:58:52 CET 2017
2017-01-25 13:53, Van Haaren, Harry:
> There was an idea (from Thomas) to better document the Acked-by and Reviewed-By in the above thread, which I think is worth doing to make the process clearer. I'll kick off a thread*, and offer to submit a patch for the documentation when a consensus is reached.
> The question that needs to be addressed is "What is the most powerful signoff to add as somebody who checked a patch?"
I do not see the benefit of knowing the most powerful.
Anyway, the most powerful tags are done by trusted people.
And people are trusted after delivering good reviews or patches ;)
The question should be "How to use the tags?"
> The documentation mentions Acked, Reviewed, and Tested by, as signoffs that can be commented on patches. The Review Process section mentions Reviewed and Tested by, but nowhere specifically states what any of these indicate.
> Offered below is my current understanding of the Acked-by; Reviewed-by; and Tested-by tags, in order of least-powerful first:
> 3) Tested-by: (least powerful)
> - Indicates having passed testing of functionality, and works as expected for Tester
> - Does NOT include full code review (instead use Reviewed by)
> - Does NOT indicate that the Tester understands architecture (instead use Acked by)
> 2) Reviewed-by:
> - Indicates having passed code-review, checkpatch and compilation testing by Reviewer
Compilation testing is done by the CI.
The reviewer must just check the results.
> - Does NOT include full testing of functionality (instead use Tested-by)
> - Does NOT indicate that the Reviewer understands architecture (instead use Acked by)
I disagree here.
The reviewer must understand the impacts of the patch.
That's why a Reviewed-by tag is really strong.
> 1) Acked-by: (most powerful)
> - Indicates Reviewed-by, but also:
A maintainer may want to approve the intent without doing a full review,
especially if he trusts the author or the reviewers.
That's why I think Acked-by should not include Reviewed-by.
If a maintainer does a full review, he should use Reviewed-by instead of Acked-by.
> - Acker understands impact to architecture (if any) and agrees with changes
> - Acker has performed runtime sanity check
Not sure about this one.
Personnaly I give some Acks without testing sometimes.
We may add a Tested-by to indicate we made some tests.
> - Requests "please merge" to maintainer
Yes, "please merge" to tree maintainer (committer).
> - Level of trust in Acked-by based on previous contributions to DPDK/networking community
The level of trust applies to any tag or comment.
> The above is a suggested interpretation, alternative interpretations welcomed.
More information about the dev