From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: Understanding of Acked-By Date: Wed, 25 Jan 2017 15:58:52 +0100 Message-ID: <2358632.GCFl4gnRC2@xps13> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org, "Yigit, Ferruh" , Igor Ryzhov , Steve Shin To: "Van Haaren, Harry" Return-path: Received: from mail-wm0-f44.google.com (mail-wm0-f44.google.com [74.125.82.44]) by dpdk.org (Postfix) with ESMTP id 60F6ADE5 for ; Wed, 25 Jan 2017 15:58:55 +0100 (CET) Received: by mail-wm0-f44.google.com with SMTP id c85so30022720wmi.1 for ; Wed, 25 Jan 2017 06:58:55 -0800 (PST) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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[1], as signoffs that can be commented on patches. The Review Process[2] 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. Thanks Harry