refactor: allow more information for pull request creation #119
Conversation
vohoanglong0107
force-pushed
the
refactor-create-with-more-information
branch
from
3393e5f to
7996aec
Compare
| @@ -30,7 +30,7 @@ pub(super) async fn command_approve( | |||
| Approver::Myself => author.username.clone(), | |||
| Approver::Specified(approver) => approver.clone(), | |||
| }; | |||
| db.approve(repo_state.repository(), pr.number, approver.as_str()) | |||
| db.approve(repo_state.repository(), pr, approver.as_str()) | |||
There was a problem hiding this comment.
I understand that we need to store more data about PRs in the database. First, I assumed that we will rely on the PR edited event to update data in our DB, and then just approve based on the PR number. But if I understand this correctly, you want to directly do PR update + approval in the DB, so that it stays atomic?
This sounds like a good idea, to avoid situations where we get a race condition and approve something else than what you see on GH. Just wanted to confirm that this was the motivation :)
There was a problem hiding this comment.
I didn't think of it like that 😅 My intention was only because we need to use get_or_create in approve, we would need the entire PR
There was a problem hiding this comment.
Well 😆 In any case, I think that using the latest information from the PR struct, since we get it from the webhook anyway, is a better idea, to be 100% sure of what commit we are approving.
There was a problem hiding this comment.
Yeah, this pr instance was retrieved from either the webhook or by ourselves using gh_client.get_pull_request so it's 100% sure that the latest data was presented when we performed approval
There was a problem hiding this comment.
Using a GH API call could actually lead to a race condition (approve -> push -> get PR from API). That's why I like reading the PR from the approval webhook directly :)
There was a problem hiding this comment.
That's a real issue.
Hmm, how can we fix it then, given that webhook payload sent to us when we approve pr (issue_comment) doesn't include the pull request information inside of it?
There was a problem hiding this comment.
Well, we can't :) I thought that the PR payload is in the webhook, but since it isn't, we just need to do the GH API call, or depend on what we have in the DB. Let's keep it as it is now and do the API call. The race condition should be rare, and on GH you can check which SHA was approved, since the bot should include it in the "approved" comment.
|
Actually, when there is push event we aren't informed of which pr the push event belonged to, only the ref of the branch, so I need to store the branch name to perform pr extraction by name |
|
I think we can use the |
|
This is the first time I've heard about |
Context
I'm working on un-approving pr whenever new commits were pushed against the branch, but it requires adding a branch column to
pull_requesttable. New pr creation will need branch information, so I made this refactor to make the changes easier