Refactor to improve synchronization and testability #45
Conversation
bryanburgers
changed the title
bryanburgers
force-pushed
the
refactor-synchronization
branch
from
bd03bec
to
6356b54
Compare
bryanburgers
force-pushed
the
refactor-synchronization
branch
from
6356b54
to
d9251e0
Compare
|
Making progress! I now have Homu synchronizing its initial state from GitHub Timeline Events in a pretty accurate way. A few differences yet where I need to track down what the actual expected state should be. Current Homu:
Local Homu with these changes:
Anybody can pull this branch and try it locally. Some notes about that:
|
Pull code that creates a comment on a pull request out of the authorization check, and add tests around the authorization checks. After pulling out the code that creates a comment, we still need to know what the text of the comment will be, so change from a function that returns a True on success and a False on failure to a function that returns a True on success and raises an Exception (with the failure comment) on failure.
Remove the free function `db_query` in favor of a `LockingDatabase` class that wraps a database connection. This is done so that, in order for a class to use the database, it only needs its `db` instance, instead of needing both a `db` instance and a `db_query` function. This allows us to break out classes into separate files.
Extract PullReqState into its own file for code readability. Because PullReqState shares some constants with main and server, extract those constants into a `consts.py` file as well.
For some critical comments, Homu adds extra information about its state
in the form of a JSON blob to the comment that isn't visible to the user
but is visible in the source for the comment. For example, Homu may
leave a comment like the following, where the JSON blob is not visible
because of the `<!-- .. -->` markdown/html comments.
⌛ Trying commit abcdef with merge 012345...
<!-- homu: {"type":"TryBuildStarted","head_sha":"abcdef","merge_sha":"012345"} -->
This change parses this extra information out of the comments and makes
it available to the initial synchronization algorithm.
Create the general structure for `process_event` and its testing, and get a long way toward testing approval comments.
Break up the "status" field into multiple orthogonal state fields: * build state (whether the primary is running or has succeeded or failed) * try state (whether the most recent try is running or has succeeded or failed) * approval state (whether the pull request is approved) Previously (and still) these were mostly possible to determine by looking at `state.get_status()` and `state.try_`, but storing them separately helps make state changes more explicit. Also, keep track of the current github synchronization cursor in the pull request state, so that we can use it later.
Test that issuing a `@bors retry` command moves the state from 'pending' to '' for pending pull requests. This is frequently used as a way to yield the current build to a different pull request.
Homu creates a message when a try or build timeout occurs. Handle this to keep the state properly updated.
bryanburgers
force-pushed
the
refactor-synchronization
branch
from
d9251e0
to
2b83596
Compare
|
Status update I've been running a "follower" against the rust-lang/rust repo using this new sync method for a couple of days now. Every minute or so, it grabs all of the PRs from the API and resyncs them from the previous sync point to current. https://homu.burgers.io/queue/rust Takeaways:
|
|
I think GitHub returns at most 100 objects from any query, so we're probably not asking for the next page on some query. |
|
Line 1495 in abd0083 Right. https://github3py.readthedocs.io/en/stable-0.9/repos.html#github3.repos.repo.Repository.iter_pulls suggests that it But at this point, I don't believe it's worth investigating. |
|
Somewhat agreed, though I think we'd want to investigate before going forward -- missing PRs in the queue are annoying because you can't easily tell if we have all of them by comparing the number, you have to check one by one. |
|
Status update: I've been incredibly busy the last couple of weeks with other things and haven't been able to get back to working on this. From what I see, this change is not something I can introduce in pieces, so it will take time to get it to a production state. I hope to be able to return to this mid-August. |
Add more of the state to the Repository class, and make each PullReqState reference it's Repository and get information from there.
Include a history of all of the tries and all of the builds when parsing the history of a pull request.
|
Slowly but surely trying to keep working on this. I now integrated a history of tries and builds, based on the GitHub history and bors' past comments, so we can have something like the following image (but with more UI work).
https://homu.burgers.io/results/rust/58281 I'll keep running https://homu.burgers.io/queue/rust while I work on this. From what I can tell checking in every once in a while, it's a pretty accurate representation of the state of the world. And is almost always more accurate with respect to mergeability and assignees. Unfortunately, it's getting long and is going to be a huge pain to review, and a huge risk to switch over. |
Previously, we discretely set `status`, `try`, `build_state`, and `try_state` on each event. With the addition of the run histories, we can now glean all of this information from those histories instead of tracking them independently. The results appear to all be identical on the current Homu queue except for one edge case: a pull request was tried, approved, then unapproved. - Using the previous method, the status would be '', because approval changes the status from 'success (try)' to '', and unapproval doesn't change the status. - Using the current method, the status would be 'success (try)', because a successful try has occured for the relevant commit hash and the PR isn't approved.
A start to the task of refactoring to improve synchronization and add more testing to Homu. More information about the target end goal can be found at the Proof-of-Concept PR #44