Hi Josep,

To clarify, there is no subjectivity as far as I can tell. The
approved-by/reviewed-by trailers would be used if you used the PR `approve`
button. The former for committers and the latter for non committers. Anyone
else who left comments would be  in the commented-by trailers. The latter
is optional, but useful to track contributions from future contributors
without incentivizing premature approvals.

Ismael

On Thu, Mar 27, 2025 at 7:53 AM Josep Prat <josep.p...@aiven.io.invalid>
wrote:

> Hi David,
> I find having the "commented" and "reviewed" distinctions a bit subjective.
> In my opinion, distinguishing between "approved-by" and "reviewed-by" is as
> far as I would go.
> Regarding the Jira trailer, I don't have any strong opinion, but it does
> help in the case of a PR working on different issues at the same time.
>
> Best,
>
> On Thu, Mar 27, 2025 at 2:50 PM David Arthur <mum...@gmail.com> wrote:
>
> > Thanks for the feedback! A few common answers first:
> >
> > I think "Approved-by" should be the only required trailer. Since
> approving
> > a PR implies a review, I think we can keep the mandatory trailers just
> to a
> > single one.
> >
> > "Co-authored-by" is added automatically by GitHub if a PR has commits
> from
> > another author. I don't think we can modify this.
> >
> > "Signed-off-by" is added automatically by GitHub if a PR has commits
> which
> > were cryptographically signed (e.g. "git commit -S"). Again, we can't
> > control this.
> >
> > For the two trailers automatically added by GitHub, if we include them
> > explicitly in the PR body they will not be added a second time by GH. For
> > an example, see
> > https://github.com/apache/kafka-merge-queue-sandbox/pull/68 and
> > the resulting commit
> >
> >
> https://github.com/apache/kafka-merge-queue-sandbox/commit/a100107be3cb7bd2256acc9552f3697a597b86e9
> > .
> > The reason we want to add them explicitly is that if we don't, GitHub
> will
> > add them automatically below a blank line which will break other
> trailers.
> >
> >
> > ---
> >
> > Ismael,
> >
> > 1) Yes, I would love to see this
> > 2) GitHub arguably uses Signed-off-by incorrectly, but it's out of our
> > control.
> >
> >
> > José,
> >
> > I would like to reserve "Approved-by" for the binding committer approval
> of
> > the PR. As Ismael suggested offline, we could use the following:
> >
> > Commented-by: left any comment on the PR (any contributor)
> > Reviewed-by: did a full review on the PR (any contributor)
> > Approved-by: committer(s) who approved the PR
> >
> > Chia,
> >
> > I personally don't find the Jira ticket in the commit subject to be very
> > useful, but this is probably a contentious opinion :) Moving it to a
> > trailer lets us reference multiple tickets or other resources so we can
> > still search by ticket number. We can leave the KAFKA-12345 in the commit
> > subject.
> >
> >
> > Kirk,
> >
> > I'd like to automate this as much as possible. I think we can eventually
> > have all of the important trailers automatically populated.
> >
> >
> >
> > -David
> >
> >
> > On Wed, Mar 26, 2025 at 3:00 PM Kirk True <k...@kirktrue.pro> wrote:
> >
> > > Hi David,
> > >
> > > In general, I'm in favor of adding information where reasonably
> possible.
> > >
> > > How are these header values populated by the merging committer? Magic
> or
> > > manual?
> > >
> > > I agree with others that adding so many additional "*-by" headers could
> > be
> > > confusing, leading to inconsistent usage. Is the equivalent of the
> > > "Signed-off-by" header already captured by git/GitHub on merge?
> > >
> > > Thanks,
> > > Kirk
> > >
> > > On Tue, Mar 25, 2025, at 5:06 PM, David Arthur wrote:
> > > > Hello Kafka community!
> > > >
> > > > I wanted to start a discussion around our Git commits and the
> metadata
> > we
> > > > keep in them.
> > > >
> > > > We have historically used the "Reviewers" Git trailer [1] to indicate
> > who
> > > > had reviewed a commit. Originally, it seems we used this field to
> > > indicate
> > > > the committer who approved the change (per our By-Laws). But over
> time,
> > > its
> > > > usage has expanded to include anyone (committer or not) who left a
> > > comment
> > > > on the PR.
> > > >
> > > > I think acknowledging reviews is very important for our community,
> and
> > I
> > > > want to continue doing this.
> > > >
> > > > I also think it is important to record the committer who approved a
> > given
> > > > PR.
> > > >
> > > > Besides improving the quality of our Git log, I'm raising this issue
> > > > because of a limitation/quirk I've discovered in GitHub. While
> > > researching
> > > > https://github.com/apache/kafka/pull/19242 I found that GitHub will
> > > > automatically re-wrap the text of the PR body to fit 72 characters.
> > This
> > > > will blindly break long "Reviewers" lines (which we regularly
> exceed).
> > > This
> > > > will make it difficult to easily find reviewers of PRs using the Git
> > log.
> > > >
> > > > I would like to propose that we start using the following Git
> trailers
> > in
> > > > our PRs:
> > > >
> > > > * Reviewed-by: anyone who left feedback on the PR
> > > > - * Approved-by: committers who approved the PR
> > > > - * Helped-by: shout-outs for inspiration or significant help
> > > > - * Signed-off-by: commit signatory
> > > > - * Fixes: KAFKA-12345 <
> > > https://issues.apache.org/jira/browse/KAFKA-12345>
> > > > - * References: KIP-123, KAFKA-23456, apache/flink#1234, etc
> > > >
> > > > Each "-by" trailer should include only a single individual using the
> > > > standard "First Last <email>" format. Multiple instances of the same
> > > > trailer are allowed. Before merging, we would use a GitHub Action to
> > > verify
> > > > that at least one "Approved-by" is present.
> > > >
> > > > This accomplishes a few things:
> > > >
> > > > * Works around the GitHub 72 character limit
> > > > * Keeps a cleaner record of reviewers vs approvers
> > > > * Allows us to move metadata out of the commit subject, if desired
> > (e.g.,
> > > > KAFKA-12345)
> > > >
> > > > Structuring the metadata in this way allows us to use standard git
> > > commands
> > > > to parse the commits. This helps us move towards a fully automated
> > > process
> > > > where we populate these fields from the GitHub Pull Request data.
> > > >
> > > > Let me know what you think!
> > > >
> > > > David Arthur
> > > >
> > > > [1] https://git-scm.com/docs/git-interpret-trailers
> > > >
> > >
> >
> >
> > --
> > David Arthur
> >
>
>
> --
> [image: Aiven] <https://www.aiven.io>
>
> *Josep Prat*
> Engineering Director, Streaming Services, *Aiven*
> josep.p...@aiven.io   |   +491715557497
> aiven.io <https://www.aiven.io>   |   <https://www.facebook.com/aivencloud
> >
>   <https://www.linkedin.com/company/aiven/>   <
> https://twitter.com/aiven_io>
> *Aiven Deutschland GmbH*
> Alexanderufer 3-7, 10117 Berlin
> Geschäftsführer: Oskari Saarenmaa, Hannu Valtonen,
> Anna Richardson, Kenneth Chen
> Amtsgericht Charlottenburg, HRB 209739 B
>

Reply via email to