[Policy] Automatically check whether 'needs review' patches apply and pass linting

I see both of these issues as being two sides of the same coin, with the other needing to happen first, along with some policy decisions to be made.

RTBC patches are simpler in that there is almost always a single patch under consideration, therefore it's simpler to run the testbots against the most recent patch, and assume that it is the patch in need of retesting

With the CNR issues, we have other problems that I mentioned in #1 here: https://www.drupal.org/node/2252787#comment-8733945. It pretty much boils down to *what is the algorithm we should use to determine which patches on an issue need to be retested?"

Automatically rerolling patches will be the first step, as making sure a patch still applies is the first thing a testbot does before proceeding. Sometimes a patch is still valid, but doesnt apply, but can after a reroll. If there is a new patch created, that will trigger the testbots to test that new, rerolled patch.

Fundamentally, what we want is for the CNR status to *mean something* and be a useful status

I totally agree, and the only obstacle to making something like this happen is sorting out how to handle all the possible situations that can arise on a CNR issue.

I believe there would be additional side benefits to this as well. If a core commit happens and breaks a CNR patch, right now nobody really knows. If this were in place it would give everybody who's involved with that patch a nudge in the form of a notification that "hey, your patch that was almost ready now needs a little work. The sooner you get it back to CNR, the less work it'll be to keep chasing HEAD."

Thanks @thedavidmeister, this is great. I've been wanting some feedback on those issues raised, and it sounds to me like we could use a heuristic of "most recent non-hidden patch" in most cases and let people manually sort out the edgier cases. That was my instinct, but its great to have some confirmation.

One reason I think automatic re-rolls should happen first is that it gets us most of the way there on this issue too. The re-rolling process would generate a new patch file, and as such would already trigger a re-test as the testbot runs on newly submitted patches. Anything that doesn't apply and cant be auto-rerolled would get marked CNW.

The only remaining 'stale' CNR patches would be ones that still manage to apply from core commit to core commit. I would hazard a guess that a stale patch that happens to still apply might also be likely to still pass tests as well. If somebody were to come along and review that patch, they would be able to apply it, examine it, and maybe set to RTBC. Once it hits RTBC it would likely not get pushed right into core that day, so it too would get another pass through the testbots.

We have some other considerations to keep in mind, namely that the testbots are not an unlimited resource and that we haven't yet flipped on 'retest all the CNR' precisely because there are a few thousand of them. But yes, even if there were just one dedicated testbot that walked serially through all the CNR's it would take a considerable amount of time, and we currently don't have a parallel queueing mechanism to even allow for that. The testbot infrastructure is undergoing an overhaul at the moment(http://www.drupal.org/project/drupalci, so there is definitely some reluctance/hesitance to add features to PIFR/PIFT - so trying to keep within the bounds of how the testbots already interact with the issue queues (namely, they test new patches) might be an easier target to hit.

Currently, testing of RTBC only patches is overwhelming our existing infrastructure, and we've had requests from core maintainers to actually STOP re-testing these issues. I believe that adding 'needs review' to the mix would only compound the problems they are running into.

Given the mixed opinions, some wanting more testing and some wanting less, I don't see us expanding to include the 'needs review' patches in the short term.

When the retest feature was implemented, there were just over 400 RTBC patches, and we enabled requeuing of 5 RTBC patches every 15 minutes.

One CNR per day would not overload the testbot ... but I'd argue that it also wouldn't provide any added benefit to the community, as the vast majority of CNR tests would never be retested.

No math to back it up ... I was basing that on your #7 statement:

the oldest 20% of all current CNR issues will be processed within a year

... which to me means that 80% (or the 'vast majority') would not be processed.

The technical problems being expressed against retesting is that the testing infrastructure has a tendency to throw false negatives ... which results in status changes on those issues, wrongly bumping those RTBC (and, with this proposal, needs review) issues down to 'needs work'. This results in people spinning cycles trying to debug or troubleshoot problems which don't actually exist.

I'd prefer spending time working on a new, more robust and feature rich testing infrastructure solution, than spending that time adding more functionality to our existing infrastructure, which is rather fragile after years of duct tape and bailer-twine fixes. I'm allowed to say that ... I'm the one who's been doing the fixing. ;)

Right now, I value stability of the current platform higher than I value new functionality ... partially due to the push towards D8, partially due to the capacity implications (The 15 minute testbots which cost us 20 cents per hour during Drupalcon Austin are now upwards of $12 per hour, so the cost/capacity equation has become a concern again), and partially due to the fact that we're in the middle of developing something new that doesn't inherit the architectural limitations built into the existing system - and any work on the old delays the launch of the new.

In the meantime, Im currently working on a solution that might get a long way towards a less messy queue. There are 1400 CNR patches in the 8.0.x Issue queue, and about 700 for 7.x. It wouldnt be terribly difficult or time consuming to sort out which of those no longer apply to HEAD. That would weed out a considerable number of patches from the CNR status.

One problem with this, however, is if we mass bump that many threads, a lot of people are going to get a lot of notifications, which will definitely stimulate a lot of activity in the queues. I just did something similar with a mass move of issues in the infrastructure queue, and a lot of activity happened as a result. So.. would that be a bad thing, or would people wonder why they suddenly got 50 emails on a bunch of old issues saying a patch no longer applies?

This will probably fall into our policy framework. Its still, very, very relevant. [#2696421]

It would be good to revisit this one. There are currently nearly 3,500 issues at needs review in the Drupal core queue. The oldest has a patch against Drupal 6 posted in 2011.

Is this still blocked on #2252787: Automatically reroll patches that do not apply (post new patch that has a successful merge with no conflicts) ?

Marking #2233265: [Policy] Frequent re-testing of RTBC/CNR patches for whether they apply and/or linting as duplicate.

With merge requests, GitLab will regularly check if they are still mergeable. That should take care of finding changes that no longer apply without conflict. This is not currently surfaced directly in the issue queue, #3178267: Update issue status on merge request activity? is one way that could be done. We could also look at putting merge status in issue queue listings.

For full testing of issues in needs review - that would have some amount of cost.

For full testing of issues in needs review - that would have some amount of cost.

Could we do it something like once every three months? Or even a one-off to see what happens to start?

I found this following @catch's article at https://www.thirdandgrove.com/insights/issue-queue-analysis/

If there is a large cost in fully testing patches, for NR patches could we only do the cheaper parts - the apply and perhaps lint steps - and then set patches that fail these to NW with the appropriate tag? Having a bot automatically tag an issue as "Needs reroll" would definitely save reviewer time.

With merge requests, GitLab will regularly check if they are still mergeable.

This unfortunately doesn't really affect things very much even if it reported back, because issues that are going stale tend to be MRs against older branches which are no-longer supported. i.e. there's a 100% chance that an MR against 9.3.x that applies now will continue to apply for the next ten years. #3294814: [Policy] Branch Naming: Use an 11.x branch for HEAD, then use 'main' when d.o can support it which moves active MRs to a 'main' branch instead would make it more meaningful, but we'd still have the 3,000 issues with MRs against old branches and patches to deal with.

How difficult would it be to run a one-off job against the curent needs review queue that checks if patches apply + linting without running tests? We currently have over 3000 issues needs review against 8+ with some not touched for seven and a half years.

Re-titling to make this just about checking whether things apply and pass linting, that would be the most help here.

Ran some things on my end:

To give an idea, out of the 2628 issues NR in core, 2385 have a patch/MR. 1299 issues have code that does not apply to 10.1.x branch.

So we have 1089 issues (45%) that can be reviewed with code that applies to the 10.1.x branch, which means 55% of NR issues should probably not be NR since the code needs to be updated.

Currently running the commit checks on the remaining 1089 issues to see how much can actually be reviewed, looks like it'll take a few hours to have a number.

got 240 commit check failures on the remaining 1089 issues, so we actually have 849 issues ready to be reviewed.

That's 35% of all the NR issues (with patch/MR)

That is dramatic. I wonder if it would be permissable to do a non-DrupalCI approach like project-update-bot to clear things out until something more permanent is in place.

My script might not be accurate because I've checked applying to 10.1.x and commit checks to 10.1.x even for issues tagged 9.5.x and when there is a patch or several MR, I'm taking the code of the latest thing so it doesn't account for issues with 2 different MR or something. Even with that I think the ratio is pretty close to the reality.

Now that I have the list I'll start going over it manually to see where are the false negatives happening and start clearing the NR queue.

There are a couple of people watching the "Needs reroll" tag, and sometimes the reroll isn't correct but it stays NR because there isn't a auto NW on lint failures so even with a non CI approach it would need to be done regularly to make sure people getting familiar with rerolls have appropriate feedback without someone pointing out the automated failures.

Even with some automation and reply template I'm going though I'm at around 10 issues per hour when looking only at the obvious ones. At this rate it would take a solid month of full time work to go through all the NR issues that are not NR (assuming nobody touches the issue queue during that month :), so any amount of time spent automating this would be worth it.

tweaked my script a bit, latest numbers.

Total issues: 2268
Issues with no patches: 65
Number of patches not applying: 1071
Number of commit check failed: 206

Attached is the log of each issue state with some logs got the commit checks failure.

So question. I agree this change will help with the needs review queue but will it drown out more modern tickets? If I go to check the latest issues which are actively being worked on and this fix runs then those active tickets will be pushed down right? Maybe not a real issue but just wanted to bring that up.

If they're actively being worked on it won't matter where they are in the overall queue.

It will turn already stale NR issue into stale NW issue. And once we know how to deal with the needs review queue we'll make a Needs work queue initiative :D

haha may need an initiative to track these initiatives.

I have high hopes that when the needs review queue/initiative gets off the ground it will have a very positive impact for the community.

deleted - wrong issue

Delete - wrong issue.

Just wanted to follow up on this one and what next steps would be needed?

Been avoiding the super old tickets as they need an actual review for relevance and is fueling credit farming while that is being done. Instead of focusing more active tickets but means the queue is staying around 2600+ (Between D9 + D10)

I think that credit farmers shouldn't be the main reason for "avoiding" reviews on very old patches. Credit farmers are present more or less everywhere and commiters have to deal with this anyway while granting credits. Probably the more relevant factor is the community focus/activity itself, as the recent active issues have better chance to get finished and fixed. But there are also interesting and valuable issues also among the very old ones.

On next steps:

It's unlikely the DA will want to spend time doing this on DrupalCI, because that work will have to be done again as part of the gitlab migration.

However, if we can get permission from the DA to create a 'patch linter' bot account, nod_ could potentially run his script as a one-off to reset the queue. This would be extremely useful even as a one-off (or a couple of one-offs).

We need to figure out what this would look like on gitlab. One problem with MRs as they currently are is that stale MRs, say against 9.4.x will never not be rebasable because their target is the now-static 9.4 branch. If we can do #3294814: [Policy] Branch Naming: Use an 11.x branch for HEAD, then use 'main' when d.o can support it then MRs would always be against the latest branch, or if against a specific branch because we're explicitly opening two branches to handle divergence.

So something like:

- MRs are always against main (unless they're backports)
- Set gitlab to auto-rebase MRs
- when auto-rebase fails, mark the issue needs work

That still leaves linting though, probably would not want that to run every rebase, but I don't know how easy it is to schedule something like that to run at arbitrary intervals against an MR.

I am willing to authorize the use of a new official bot for this purpose.

The parameters would be that we follow the best practices of the project update bot:

• It has a way for the maintainer of any given project to opt out, for example. (In this case, we'd want to let someone opt out an individual issue perhaps).
• It also needs to be easily tunable for the amount of re-testing it's doing. The price of testing has gone up lately. It can vary between 75 cents and 1.20 an hour now.
• It should have a dedicated account, with a user description that explains what it is doing and where to file issues.
• Messages it posts should be clearly documented in issues, so that people can provide feedback.
• Lastly, it'd be a great help if someone could help capture any other known standards we follow for project update, and take those and the ones above, and help me create a new page under the Drupalorg documentation guide for 'bot standards'. It's something I've been meaning today for more than a year (and eventually will, but help is welcome).

That said - it wouldn't hurt for someone to check out the DrupalCI_testbot/PIFT codebase - just in case this turns out to be *really easy* to do using the existing RTBC logic. We'd certainly accept an MR.

It also needs to be easily tunable for the amount of re-testing it's doing. The price of testing has gone up lately. It can vary between 75 cents and 1.20 an hour now.

If we use nod_'s script this would be a non-issue - it does the patch linting checks on the machine it's run on.

It has a way for the maintainer of any given project to opt out, for example. (In this case, we'd want to let someone opt out an individual issue perhaps).

This would only run against core. If we're only doing it as a one-off, then it seems a bit tricky to offer an opt-out, maybe we could respect a special issue tag though?

@Catch - sure - if we're limiting the scope in those ways, than that can serve in place of having those configuration options. Makes it easier certainly.

(Useful stuff described in our existing bot account: https://www.drupal.org/u/project-update-bot)

Planning on running my script on tuesday 31st. What does the script does:

1. Get all Needs Review issues for major versions: 11, 10, 9
2. For each issue:
   • Find the last patch, either the most recent patch file upload or the most recent merge request. If there are patches and merge request on the same issue, use the most recent.
   • Download the patch or the diff from the merge request
   • Apply the patch locally on the corresponding major version (10.1.x for issues with 10* version, 9.5.x for issues with 9* version)
   • If the patch applies, run the commit checks
3. If the patch didn't apply or commit check failed, put the issue back in Needs Work with a message explaining what's going on. I'll try to look at submitting a file with the result of the patch apply or the commit check log.

FYI I get a 5xx error when trying to look at https://www.drupal.org/u/project-update-bot

The page loads for me after a while, I think it's the 3,500 issue credits taking a while.

Copy and pasting from there:

This account was created by the Drupal Association to help upgrade contributed projects to Drupal 9 and then to Drupal 10. Currently it provides patches generated by Drupal Rector to remove Drupal 10 deprecated API uses. All of those issues can be found at https://www.drupal.org/project/issues/search?issue_tags=ProjectUpdateBotD10

If you are a project maintainers there are a few options for dealing with issues created by this bot:

Leave the issue open and apply the provided patch to remove some or all Drupal 10 deprecated API uses. The Project Update Bot will check weekly if Drupal Rector is able to remove new deprecations and post a new patch if possible.
Remove the “ProjectUpdateBotD10” tag from the issue to stop new patches from being posted. If you would like to use the issue and the patch as a starting point simply remove this tag and the bot will not post any new patches. Add the tag back and the bot will post new patches if possible.
Close the issue to stop the bot from posting new patches. If you are already handling deprecated API uses in another issue or otherwise don’t find the patches helpful simply close the issue and the bot will not post any new patches.

If want to respect a tag, something like NoNeedsReviewBot might work - although really that's only going to be relevant if we do a second run. I can try to draft some copy for this tomorrow.

Rough drafts for verbiage added to the issue summary - one for the user account, one for the issue comment.

Excellent, thanks

Nice rough drafts. I suggest adding to the comment a link to the Contributor Task. "Consult the Drupal Contributor Guide to find step-by-step guides for working with issues".

Some of the sentences are long and complicated so I made an attempt to convert the content to plain English. There is room for improvement but maybe it is of some use. Here are the results:

User profile page

The Needs Review Queue Bot is a project of the Needs Review Queue Initiative. Join the Drupal Slack channel, #needs-review-queue-initiative, to find out more about the initiative. The initiative goal is to reduce the number of issues needing a review, which was 2600 when the initiative started.

The Needs Review Queue Bot performs limited testing on issues marked "Needs review". The Drupal CI does not automatically test these issue. One reason is that the cost of doing a full test run on these issues every 48 hours would be prohibitive.

The Needs Review Queue Bot only tests if a patch or MR applies to the issue target branch and runs Drupal's commit checks. If either of these steps fails, the bot will add a comment and mark the issue "Needs work".

Some issues need to stay at "Needs review" despite the existing patch or MR being out of date. In these cases, add the tag "no-needs-review-bot". The Needs Review Queue Bot ignores issues with the tag, "no-needs-review-bot".

This is a temporary measure. We hope that DrupalCI or GitLab CI adds similar automated re-linting for needs review issues.

Issue comment

The Needs Review Queue Bot tested this issue. It either no longer applies to the branch of Drupal core this issue is against, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I'm all set up now, used the text from #49 with the edit from #50 for the issue comment.

If we're ok with the text, now it's just a matter of deciding when to run things on the 1045 issues concerned. It should take about 6 hours.

The bot will run in about 4 hours

The script started, it should take about 5 hours to run provided the bot doesn't get blocked by d.o security set up :)

To see all the issues impacted by the bot you can check look at this issue list: https://www.drupal.org/project/issues/search/drupal?project_issue_follow...

Total issues: 2167
Issues with no patches: 43
Number of patches not applying: 802
Number of commit check failed: 176

Hi team, love the effort going into reducing the queue!

A little feedback on the bot that might a bit too little too late: when the patch doesn't apply, the txt file doesn't really make that clear by just including the patch link in a json format. I was pretty confused by it and predict the same for plenty of contributors.

Just leaving this here in case the bot becomes a regular fixture :)

Seconding @darvanen above. Also it would be nice to change the comment based on the failure too.

I lost a bit of time on the issues I follow today trying to figure out which failure each issue was being pushed back on (i.e failed to apply or linting).

If the contents of the file change based on that then hopefully it wouldn't be much work to change the comment too.

Thanks for the great work as always :)

Script finished properly. It did break email notifications for a while. Review Queue is at 1182 and 976 issues were put back in NW by the bot.

If we run the bot again I'll need to make it honor the needs-review tag and fix up the text of the issue. You can see what type of failure is it by the size of the text file. if it's aournd 100-ish bytes it's that the patch doesn't apply, bigger than that you have commit check failures.

With gitlab make major jumps recently, wonder if this can be closed?

I bet no, it still needs some bot to schedule jobs in the latest pipeline or to create new pipeline

https://docs.gitlab.com/ee/ci/pipelines/schedules.html

I opened a couple of gitlab-specific issues about this one: #3387120: [PP-1] MR retesting with GitLab CI and #3387147: Mark issues needs work when MRs can't be merged or pipelines fail, mostly about RTBC issues.

I think the equivalent of this issue on gitlab might be something like https://gitlab.com/gitlab-org/gitlab-foss/-/issues/18834, which would in turn trigger a new test run on the MR.

With merge requests, the equivalent to “no longer applies” would be “needs manual rebase.” GitLab and Drupal.org now show the MR’s ability to merge status prominently, for example, “Related merge requests” at https://gitlab.com/gitlab-org/gitlab/-/issues/423114. However, I don’t see anything in GitLab’s issue listing, https://gitlab.com/gitlab-org/gitlab/-/issues/?sort=updated_desc&state=o.... The merge request listing does surface the pipeline results, but I don’t see a filter for it, https://git.drupalcode.org/project/drupal/-/merge_requests

At some point, maintainers may want to consider if they want to look at the issues or merge requests listing primarily for code review. I suspect it may stay as issues, since policies and plans don’t land as code merges.

https://gitlab.com/gitlab-org/ruby/gems/gitlab-triage is what GitLab (the company) runs on top of GitLab (the software) for automated labeling. I don't see pipeline status documented as a condition.

With patches now gone should this be closed?

I do not think that currently the failing MRs or MRs which needs to be rebased, are moved to NW automatically (except by NR queue bot). Probably this is more relevant issue: #3387147: Mark issues needs work when MRs can't be merged or pipelines fail, but since this is a policy issue, I am not sure we should close this without a decision.