Maybe Sebastian is right; it's a rare case, so if that happens, we can just re-enable the ability to dismiss reviews to unblock that PR, but keep it off in general?
On Thu, Oct 2, 2025, 9:33 AM Sebastien Lorquet <[email protected]> wrote: > 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 > > >> > >
