Hi,
yes, This globally sounds like a non-problem to me.
Sebastien
On 10/2/25 15:22, Alan C. Assis wrote:
No, I mean the extreme case where someone added a Requested Change,
the change was made, but the author of the request disappeared.
So, in this case, the only option is the patch author to open a new
PR, but all the review history will be lost (not sure if this history
review is so important anyway).
BR,
Alan
On Thu, Oct 2, 2025 at 10:12 AM Sebastien Lorquet
<[email protected]> wrote:
Hello,
I think this is ok
If someone wants their PR merged they have to work on it otherwise
it is
forgotten.
Development is *never* that urgent, delays for merging are normal and
acceptable.
Sebastien
On 10/2/25 13:24, Alan C. Assis wrote:
> I think maybe it could be possible to configure github to not
allow other
> committer to dismiss the Change Request, but it has a
drawback/danger:
>
> Imagine someone added a Request Change and never came back to
remove the
> change request, the PR could be stuck there forever.
>
> BR,
>
> Alan
>
> On Wed, Oct 1, 2025 at 2:13 PM Matteo Golin
<[email protected]> wrote:
>
>> Hello everyone,
>>
>> I have noticed a large number of PRs have been getting merged,
even though
>> they do not necessarily meet the contributing guidelines that
we had voted
>> on back in March:
>> https://www.mail-archive.com/[email protected]/msg12795.html
>>
>> The major issue I have spotted is PRs with no proof of testing,
no logs and
>> no information about the test cases used being merged. I have
seen that
>> some reviewers comment on this lack of information, but most of
us seem to
>> just use the "comment" feature on reviews. I think that if you are
>> requesting that more information be included in the PR
description, or
>> really any changes, you should use the "request changes"
option, which
>> prevents the PR from being merged until the changes are made.
However,
>> there have unfortunately been cases where the review comments
have been
>> ignored and the PR merged anyways, and even cases where
"requests for
>> changes" are dismissed and the PR merged.
>>
>> I think we need to be reminded that we voted in favour of a
zero-trust
>> approach to user/author testing, and that it is the author's
responsibility
>> to provide build and runtime logs for real world hardware. We
also voted
>> for breaking changes to be validated on various real-world
devices with
>> runtime logs, and 4 independent approvals on the PR. QEMU is
not considered
>> valid for this explicitly. Obviously there are cases where some
testing may
>> not be required (i.e. fixing typos in comment strings), but the
user should
>> note that in their PR's testing section.
>>
>> I propose that we include some kind of automation (similar to
how Lup had
>> the auto-PR reviewer a while back) which just comments these
contribution
>> guidelines on the PRs themselves. I don't really know how we
can be more
>> explicit towards contributors (although suggestions are
welcome) since our
>> guidelines are very clear and the PR template itself states the
>> requirements, which are often being ignored. I am mainly trying
to resolve
>> reviewers forgetting about these guidelines so we can prevent
these merges
>> of non-compliant PRs from taking place. Usually it is just a
small change
>> required by the author (re-running their test to capture the
log output and
>> include it). I think that it's very important that we tackle
this issue,
>> because we've had multiple complaints on this mailing list now
about
>> regressions being introduced and PRs being merged with too much
haste.
>>
>> I am looking for input from the community about what we can do
to prevent
>> these kinds of PRs from getting merged and facilitate the
review process so
>> that reviewers are actually following the agreed upon
guidelines (I know
>> that there are sometimes many to remember).
>>
>> Best,
>> Matteo
>>
