Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better tracking for regressions and regression fixes #10

Open
pietroalbini opened this issue May 7, 2021 · 7 comments
Open

Better tracking for regressions and regression fixes #10

pietroalbini opened this issue May 7, 2021 · 7 comments

Comments

@pietroalbini
Copy link
Member

pietroalbini commented May 7, 2021

Editing by Mark:

We have frequently struggled with identifying and tracking regressions/fixes; this issue attempts to start the process of identifying what's needed from such a solution so that we can move forward

  • Issue filed
  • Categorized as regression in some channel, labeled regression-from-stable-to-X
  • Issue investigated, fix identified, PR filed against master branch
  • PR nominated for beta backport
  • PR approved for beta backport
  • Beta rollup created, including the PR
  • PR lands on master
  • PR lands on beta via rollup

Note that the last two steps here frequently happen in the opposite order, though not always. It's also not uncommon that the fixes on beta and master originate from slightly different patches, so that should be kept in mind too.

Our goals are:

  • For a given regression issue, quickly assess which versions it's been fixed in already, which versions a fix is pending for - ideally with links to that work
  • For a given version, identify regressions in that version (i.e. what new bugs/regressions added)
    • This is both for e.g. current beta, to track making sure we get this to "zero", but also ideally historically we want to see if this number is going up/down/sideways
@pietroalbini
Copy link
Member Author

A possible implementation that came to mind is to rely on the "linked pull requests" feature and the milestone automatically added by triagebot to track fixes across multiple versions.


Based on the milestones triagebot will keep a table updated in the issue body that tracks the fixes across versions:

Version Channel State Linked PR
1.25 stable First affected release -
🔴 1.52 stable No linked fix -
🟡 1.53 beta Fix in progress #123
🟢 1.54 nightly Fix landed! #122
Version Channel State Linked PR
🔴 1.53 beta No linked fix -
🟢 1.54 nightly Fix landed! #122

It's not possible to manually update the table. Instead, the table is populated through the "linked pull requests" feature, either by adding fixes #123 in the PR body or by manually clicking the gear icon in the issue sidebar. For open PRs triagebot will categorize the PR based on the branch it targets, and for closed PRs it will categorize the PR based on the PR milestone. This should transparently handle PRs merged across channel promotion.

As the "linked pull requests" feature closes the issue once one of the linked PRs is merged, triagebot will always reopen issues labeled regression without all the fixes, leaving a comment every time on why it did so (and what releases are not fixed). It will also close the issue automatically once all regressions are fixed. If the PR needs to be closed anyway the regression label can be removed.


On the impl side, while there is no direct API to get all the linked PRs of an issue, it's possible to get a list of all the times a PR was connected or disconnected to an issue and calculate the final state with that:

query { 
  repository(owner: "rust-lang", name: "rust") {
    issue(number: 85042) {
      timelineItems(first: 100, itemTypes: [CONNECTED_EVENT, DISCONNECTED_EVENT]) {
        nodes {
          __typename
          ... on ConnectedEvent {
            subject {
              ... on PullRequest {
                number
          } } }
          ... on DisconnectedEvent {
            subject {
              ... on PullRequest {
                number
} } } } } } } }

@Mark-Simulacrum
Copy link
Member

I updated the issue description with some initial thinking on goals, obviously we can iterate.

I think the linked pull requests are OK, but I don't think they're actually necessary? In particular requiring to match the GitHub syntax for the "fixes" or whatever and dealing with reopening seems like a pretty large pain point, because e.g. the beta-targeted rollup PR needing to enumerate those fixes is annoying (adds quite a bit of overhead). I'd suggest we keep the state in the triagebot database and then we can apply the mapping from there. It also lets us be more intelligent about the tables - for example, we currently manually update milestones after beta-merging a PR, but that's hard to get right, we'd ideally want to have automation do it for us (precisely on the merge event). I think the tying these tables would need to do that well also match well towards fixing that.

@pietroalbini
Copy link
Member Author

Hmm, we still need some metadata in the beta rollup PRs to signal what regressions they fix, right?

Also, it's not required to use fixes #123, it can be mapped after the fact in the UI.

@Mark-Simulacrum
Copy link
Member

Beta rollup PRs (and I'd argue any beta-targeted PR, per our other suggested policy of "must land on master") could include some standard template language, e.g., like I use lists for: rust-lang/rust#84996 -- you could imagine that we can make that list parsed by triagebot. I find creating that list much easier to do than clicking in GH's UI, personally, and we can have a @rustbot merged #xxx in #xxx command for adding more, or so.

@Mark-Simulacrum
Copy link
Member

I think if we're to make this work it needs to be compatible with it not needing human clicking in the UI, basically, and ideally as little work as possible on the human side - ultimately, no one doing backport rollups today has a lot of time.

@pnkfelix
Copy link
Member

pnkfelix commented Jun 2, 2021

Hey @pietroalbini when you wrote:

It's not possible to manually update the table

did you mean you would somehow make the table in the markdown source for the description not editable? Or if someone did edit the table in the description, triagebot would respond be reconstructing the table thus undoing their edits?

Or did you just mean that people should be heavily discouraged from manually editing the table (perhaps via <!-- DO NOT MANUALLY EDIT --> comments above and below the table in the markdown source?)

@pietroalbini
Copy link
Member Author

@pnkfelix I meant that changes are ignored by triagebot.

@Mark-Simulacrum I'm open to have additional ways to mark releases as fixed in addition to linked PRs. Still, support for linked PRs would allow an optional UI to change things and would make "fixed #123" work out of the box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants