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.

Sign up for GitHub

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

Jump to bottom

refactor: allow more information for pull request creation #119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: allow more information for pull request creation #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions src/bors/handlers/review.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Contributor

@Kobzol Kobzol Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

@vohoanglong0107 vohoanglong0107 Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.await?;
handle_label_trigger(&repo_state, pr.number, LabelTrigger::Approved).await?;
notify_of_approval(&repo_state, pr, approver.as_str()).await
Expand All @@ -49,7 +49,7 @@ pub(super) async fn command_unapprove(
deny_unapprove_request(&repo_state, pr, author).await?;
return Ok(());
};
db.unapprove(repo_state.repository(), pr.number).await?;
db.unapprove(repo_state.repository(), pr).await?;
handle_label_trigger(&repo_state, pr.number, LabelTrigger::Unapproved).await?;
notify_of_unapproval(&repo_state, pr).await
}
Expand All @@ -65,16 +65,16 @@ pub(super) async fn handle_pull_request_edited(
};

let pr_model = db
.get_or_create_pull_request(repo_state.repository(), payload.pull_request.number)
.get_or_create_pull_request(repo_state.repository(), &payload.pull_request)
.await?;
if pr_model.approved_by.is_none() {
return Ok(());
}

let pr_number = payload.pull_request.number;
db.unapprove(repo_state.repository(), pr_number).await?;
handle_label_trigger(&repo_state, pr_number, LabelTrigger::Unapproved).await?;
notify_of_edited_pr(&repo_state, pr_number, &payload.pull_request.base.name).await
let pr = &payload.pull_request;
db.unapprove(repo_state.repository(), pr).await?;
handle_label_trigger(&repo_state, pr.number, LabelTrigger::Unapproved).await?;
notify_of_edited_pr(&repo_state, pr.number, &payload.pull_request.base.name).await
}

fn sufficient_approve_permission(repo: Arc<RepositoryState>, author: &GithubUser) -> bool {
Expand Down Expand Up @@ -421,8 +421,9 @@ approve = ["+approved"]
) {
let pr_in_db = tester
.db()
.get_or_create_pull_request(&default_repo_name(), pr_number)
.get_pull_request(&default_repo_name(), pr_number)
.await
.unwrap()
.unwrap();
assert_eq!(pr_in_db.approved_by, Some(approved_by.to_string()));
let repo = tester.default_repo();
Expand All @@ -433,8 +434,9 @@ approve = ["+approved"]
async fn check_pr_unapproved(tester: &BorsTester, pr_number: PullRequestNumber) {
let pr_in_db = tester
.db()
.get_or_create_pull_request(&default_repo_name(), pr_number)
.get_pull_request(&default_repo_name(), pr_number)
.await
.unwrap()
.unwrap();
assert_eq!(pr_in_db.approved_by, None);
let repo = tester.default_repo();
Expand Down
12 changes: 5 additions & 7 deletions src/bors/handlers/trybuild.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub(super) async fn command_try_build(
}

let pr_model = db
.get_or_create_pull_request(repo.client.repository(), pr.number)
.get_or_create_pull_request(repo.client.repository(), pr)
.await
.context("Cannot find or create PR")?;

Expand Down Expand Up @@ -152,7 +152,7 @@ pub(super) async fn command_try_cancel(

let pr_number: PullRequestNumber = pr.number;
let pr = db
.get_or_create_pull_request(repo.client.repository(), pr_number)
.get_or_create_pull_request(repo.client.repository(), pr)
.await?;

let Some(build) = get_pending_build(pr) else {
Expand Down Expand Up @@ -638,11 +638,9 @@ mod tests {
tester.expect_comments(1).await;
let pr = tester
.db()
.get_or_create_pull_request(
&default_repo_name(),
PullRequestNumber(default_pr_number()),
)
.await?;
.get_pull_request(&default_repo_name(), PullRequestNumber(default_pr_number()))
.await?
.unwrap();
assert_eq!(pr.try_build.unwrap().status, BuildStatus::Cancelled);
Ok(tester)
})
Expand Down
33 changes: 20 additions & 13 deletions src/database/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ use sqlx::PgPool;
use crate::database::{
BuildModel, BuildStatus, PullRequestModel, WorkflowModel, WorkflowStatus, WorkflowType,
};
use crate::github::{CommitSha, GithubRepoName, PullRequest};

#[cfg(test)]
use crate::github::PullRequestNumber;
use crate::github::{CommitSha, GithubRepoName};

use super::operations::{
approve_pull_request, create_build, create_pull_request, create_workflow, find_build,
Expand All @@ -27,32 +29,28 @@ impl PgDbClient {
pub async fn approve(
&self,
repo: &GithubRepoName,
pr_number: PullRequestNumber,
pr: &PullRequest,
approver: &str,
) -> anyhow::Result<()> {
let pr = self.get_or_create_pull_request(repo, pr_number).await?;
let pr = self.get_or_create_pull_request(repo, pr).await?;
approve_pull_request(&self.pool, pr.id, approver).await
}

pub async fn unapprove(
&self,
repo: &GithubRepoName,
pr_number: PullRequestNumber,
) -> anyhow::Result<()> {
let pr = self.get_or_create_pull_request(repo, pr_number).await?;
pub async fn unapprove(&self, repo: &GithubRepoName, pr: &PullRequest) -> anyhow::Result<()> {
let pr = self.get_or_create_pull_request(repo, pr).await?;
unapprove_pull_request(&self.pool, pr.id).await
}

pub async fn get_or_create_pull_request(
&self,
repo: &GithubRepoName,
pr_number: PullRequestNumber,
pr: &PullRequest,
) -> anyhow::Result<PullRequestModel> {
if let Some(pr) = get_pull_request(&self.pool, repo, pr_number).await? {
if let Some(pr) = get_pull_request(&self.pool, repo, pr.number).await? {
return Ok(pr);
}
create_pull_request(&self.pool, repo, pr_number).await?;
let pr = get_pull_request(&self.pool, repo, pr_number)
create_pull_request(&self.pool, repo, pr).await?;
let pr = get_pull_request(&self.pool, repo, pr.number)
.await?
.expect("PR not found after creation");

Expand Down Expand Up @@ -140,4 +138,13 @@ impl PgDbClient {
) -> anyhow::Result<Vec<WorkflowModel>> {
get_workflows_for_build(&self.pool, build.id).await
}

#[cfg(test)]
pub(crate) async fn get_pull_request(
&self,
repo: &GithubRepoName,
pr_number: PullRequestNumber,
) -> anyhow::Result<Option<PullRequestModel>> {
get_pull_request(&self.pool, repo, pr_number).await
}
}
5 changes: 3 additions & 2 deletions src/database/operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::database::BuildStatus;
use crate::database::WorkflowModel;
use crate::github::CommitSha;
use crate::github::GithubRepoName;
use crate::github::PullRequest;
use crate::github::PullRequestNumber;

use super::BuildModel;
Expand Down Expand Up @@ -56,12 +57,12 @@ WHERE pr.repository = $1
pub(crate) async fn create_pull_request(
executor: impl PgExecutor<'_>,
repo: &GithubRepoName,
pr_number: PullRequestNumber,
pr: &PullRequest,
) -> anyhow::Result<()> {
sqlx::query!(
"INSERT INTO pull_request (repository, number) VALUES ($1, $2) ON CONFLICT DO NOTHING",
repo.to_string(),
pr_number.0 as i32
pr.number.0 as i32
)
.execute(executor)
.await?;
Expand Down
Loading