Repository navigation
Consider no longer using the PR consistency checking script for astropy core (or rethink) #176
Description
While working on doing a v5.0.5 release I have noticed that the PR consistency scripts now return a lot of issues that are not issues:
- When maintaining 5.0.x and 5.1.x only the changelog rendering from 5.1.x gets forward-ported to main, so e.g. a PR merged labelled as 5.0.4 will actually not appear in the 5.0.4 section of the changelog if it was also backported to 5.1.x, but instead in the next 5.1.x release (we were ok with this decision back when we set up towncrier, I just mean that the scripts here don't recognize that).
- The changelog rendering happens during the release process and so the scripts can't easily check right now the rendered changelog until the last minute - currently this causes a lot of warnings about missing changelog entries.
In addition I spent a while just adding manual merges to the script, but haven't actually encountered any real issues.
Now that we have a pretty robust system for backporting and generating the changelog, and that we check for changelog entries being present in PRs at the time when the PR is open, and furthermore that we can just check for PRs labelled 'Still Needs Manual Backport' I don't think the scripts here are really needed anymore (for the core package).
Would we be happy to switch for astropy core to just saying that one should check for PRs still requiring manual merging? This would drastically reduce the burden of doing bugfix releases.
If we want to continue using the scripts, we are going to have to rethink how the checks are done re: LTS releases and so on.
Activity
pllim commentedon Oct 20, 2022
Let's break this down and see...
Current situation:
Possible scenarios (might not be exhaustive):
pllim commentedon Oct 20, 2022
Also... sometimes people backport by pushing it directly to the backport branch, right? Instead of opening PRs.
saimn commentedon Oct 20, 2022
I used the PR consistency scripts for the 5.1 release, both to see what it was doing and to cross check that the changelog was ok. It was useful to see that everything was consistent, but I could have done without it.
The issue is that it is not so easy to check that all PRs have been backported, because of the issues mentioned by @pllim above. But I'm not sure if there is an easy way to improve that, and we could also choose to do releases with the PRs that have been backported and that's it.
bsipocz commentedon Oct 20, 2022
That's a valid idea, but in that case, their milestones must be updated to reflect that the PRs were, in fact, not backported.
astrofrog commentedon Oct 20, 2022
I guess at the end of the day:
So at release time it should be sufficient to check for:
and that should be sufficient? Then the only real failure mode I can think of is if someone removes the 'Still Needs Manual Backport' label when in fact the backport was not done correctly or only one of two was done. So the question is whether we want to find a way to guard against that - but the issue is that as they are at the moment, the scripts don't handle manual merges well unless we explicitly list them which is time consuming.
In any case, checking the changelog (the two main issues I mentioned at the start of this issue) is something that doesn't work well anymore because of how we handle LTS but the flip side is that because of the towncrier workflow it's very unlikely that there would be an inconsistency between what is backported and what ends up in the changelog so that consistency doesn't need to be checked.
saimn commentedon Oct 21, 2022
Seems a good summary, I agree that with a query on the labels and milestone we should be able to spot issues easily. The only caveat is that when backporting to both 5.0.x and 5.1.x, the PR backport for 5.1.x will have 5.0.x as milestone (because it inherits the milestone from the parent PR). It's easy to fix manually when checking the list of PRs in the milestone page.
pllim commentedon Oct 21, 2022
As @bsipocz pointed out above, we still need a script to check if the original PR milestoned to be backported need the milestone changed if the backport somehow didn't make it to the actual bugfix release?
Yeah, at first I tried to manually correct that but then I gave up because it was too much work to keep track especially when other maintainers also merging them without changing inherited milestone. 😅
pllim commentedon Feb 28, 2023
At 2023-02-28 infrastructure tag-up, those present expressed a need to brainstorm more on the alternatives first.