Another point of clarification, I think we should reserve the Approved-by for the committer(s) who approved the PR. If a non-committer approves a PR, I think it should appear as Reviewed-by.
-David On Thu, Mar 27, 2025 at 5:58 PM Ismael Juma <m...@ismaeljuma.com> wrote: > 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 > > > -- David Arthur
