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

x/build/maintner: Maintner does not detect transferred issues #37370

Open
jmdobry opened this issue Feb 22, 2020 · 3 comments
Open

x/build/maintner: Maintner does not detect transferred issues #37370

jmdobry opened this issue Feb 22, 2020 · 3 comments
Labels
Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jmdobry
Copy link

jmdobry commented Feb 22, 2020

Related to #30184.

Maintner's sync loop uses a ListIssues call sorted by update time to determine which issues it needs to re-fetch issue data and event data for. Deleted issues and transferred issues don't show up in the ListIssues response, Maintner is never aware that they're gone, and ultimately those issues are frozen in the corpus in the state they had before they were deleted/transferred. They are never marked NotExist (tombstoned). https://go-review.googlesource.com/c/build/+/161521/ is an insufficient fix for this reason.

https://go-review.googlesource.com/c/build/+/205598 proposes exposing a mutation method so an external process can inform Maintner of the deleted/transferred issues, but that requires an external process (also, see other comments on that CL).

I think a better solution would be for Maintner, during its issue sync, to pull repository events up to the GitHub poller's lastUpdated timestamp, looking specifically for issue transfer and issue delete events, and tombstoning (mark as NotExist) the appropriate issues before moving onto the regular issue sync.

The difficulty will be for repos that are already being synced, this doesn't provide a way to go back in time and handle previous deletes/transfers, which, the external process would be capable of doing.

/cc @bradfitz @dmitshur @orthros

@gopherbot gopherbot added this to the Unreleased milestone Feb 22, 2020
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Feb 22, 2020
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 22, 2020
@jmdobry
Copy link
Author

jmdobry commented Feb 22, 2020

I implemented my suggestion, and discovered that GitHub doesn't actually add deleted and transferred events to the originating repository's issue events, rendering this idea a non-starter.

Instead, I'm reverting back to relying on https://golang.org/cl/161521/ as a partial fix.

@stamblerre
Copy link
Contributor

stamblerre commented Apr 9, 2021

@dmitshur has suggested that we could check the endpoint https://api.github.com/repos/golang/vscode-go/issues/144/events (for an issue like https://github.com/golang/vscode-go/issues/144). Then, we could just skip adding any issue that no longer exists.

@gopherbot
Copy link

gopherbot commented Mar 31, 2022

Change https://go.dev/cl/397234 mentions this issue: maintner: mark issues as NotExist when its comments are gone

gopherbot pushed a commit to golang/build that referenced this issue Apr 1, 2022
When synchronizing comments of an issue, getting a 404 (or 410) from
GitHub REST API's list comments endpoint should happen if and only if
the issue no longer exists. Use that as a safe opportunity to append
a mutation to the log that updates the issue's NotExist field to true.

Maintner doesn't yet have complete support for transferred issues,
a feature that GitHub added after its initial design. It's been
difficult to add it retroactively as described in go.dev/issue/37603.

This change aims to make a small incremental change that we can feel
confident about applying and watching closely, and doesn't try to
implement a complete fix. It should help get maintner unstuck when
trying to sync comments in an issue tracker that has a transferred
issue, and incurs a minimal risk of writing bad mutations.

This CL is based on past work in CL 161521 and CL 176638,
but is smaller in scope intentionally.

For golang/go#37370.
For golang/go#40640.
Updates golang/go#45461.
Fixes golang/go#52017.

Change-Id: I3702d3c7b2e412cf75809fdb722c0ee693c8600e
Reviewed-on: https://go-review.googlesource.com/c/build/+/397234
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants