From 8494566f63a1920e2b06ca4048190c0acda9f8af Mon Sep 17 00:00:00 2001 From: Troy Benson Date: Fri, 3 Jan 2025 21:03:25 +0000 Subject: [PATCH 1/2] wd --- .github/brawl.toml | 4 - server/src/command/dry_run.rs | 8 +- server/src/command/merge.rs | 8 +- server/src/command/ping.rs | 10 +- server/src/command/retry.rs | 4 +- server/src/github/config.rs | 25 +-- server/src/github/installation.rs | 144 +++----------- server/src/github/label_state.rs | 8 +- server/src/github/merge_workflow.rs | 29 ++- server/src/github/mock/get_ref.json | 10 + server/src/github/mod.rs | 14 +- server/src/github/models.rs | 3 + server/src/github/repo.rs | 294 +++++++++++++++++++++++----- server/src/webhook/check_event.rs | 7 +- 14 files changed, 343 insertions(+), 225 deletions(-) create mode 100644 server/src/github/mock/get_ref.json diff --git a/.github/brawl.toml b/.github/brawl.toml index 3a4f45b..ad0fd11 100644 --- a/.github/brawl.toml +++ b/.github/brawl.toml @@ -1,9 +1,5 @@ enabled = true -branches = [ - "main", -] - merge_permissions = [ "role:write", ] diff --git a/server/src/command/dry_run.rs b/server/src/command/dry_run.rs index 703ba48..f6e69bd 100644 --- a/server/src/command/dry_run.rs +++ b/server/src/command/dry_run.rs @@ -33,7 +33,11 @@ async fn handle_with_pr( context: BrawlCommandContext<'_, R>, mut command: DryRunCommand, ) -> anyhow::Result<()> { - if !context.repo.config().enabled { + let Some(config) = context.repo.config().await? else { + return Ok(()); + }; + + if !config.enabled { return Ok(()); } @@ -92,7 +96,7 @@ async fn handle_with_pr( .map(Base::from_sha) .unwrap_or_else(|| Base::from_pr(&pr)); - let branch = context.repo.config().try_branch(pr.number); + let branch = config.try_branch(pr.number); let db_pr = Pr::new(&pr, context.user.id, context.repo.id()) .upsert() diff --git a/server/src/command/merge.rs b/server/src/command/merge.rs index f5128a4..067cc24 100644 --- a/server/src/command/merge.rs +++ b/server/src/command/merge.rs @@ -33,7 +33,11 @@ async fn handle_with_pr( context: BrawlCommandContext<'_, R>, command: MergeCommand, ) -> anyhow::Result<()> { - if !context.repo.config().enabled { + let Some(config) = context.repo.config().await? else { + return Ok(()); + }; + + if !config.enabled { return Ok(()); } @@ -110,7 +114,7 @@ async fn handle_with_pr( let run = CiRun::insert(context.repo.id(), pr.number) .base_ref(Base::from_pr(&pr)) .head_commit_sha(pr.head.sha.as_str().into()) - .ci_branch(context.repo.config().merge_branch(&pr.base.ref_field).into()) + .ci_branch(config.merge_branch(&pr.base.ref_field).into()) .maybe_priority(command.priority.or(db_pr.default_priority)) .requested_by_id(context.user.id.0 as i64) .approved_by_ids(reviewer_ids) diff --git a/server/src/command/ping.rs b/server/src/command/ping.rs index 9310619..fb23675 100644 --- a/server/src/command/ping.rs +++ b/server/src/command/ping.rs @@ -8,6 +8,8 @@ pub async fn handle( _: &mut AsyncPgConnection, context: BrawlCommandContext<'_, R>, ) -> anyhow::Result<()> { + let config = context.repo.config().await?; + // Should we also say what permissions the user has? context .repo @@ -15,7 +17,7 @@ pub async fn handle( context.pr_number, &messages::pong( context.user.login, - if context.repo.config().enabled { + if config.is_some_and(|c| c.enabled) { "enabled" } else { "disabled" @@ -35,6 +37,7 @@ mod tests { use super::*; use crate::command::BrawlCommand; use crate::database::get_test_connection; + use crate::github::config::GitHubBrawlRepoConfig; use crate::github::merge_workflow::GitHubMergeWorkflow; use crate::github::models::User; use crate::github::repo::test_utils::{MockRepoAction, MockRepoClient}; @@ -87,7 +90,10 @@ mod tests { let mut conn = get_test_connection().await; let (mut client, mut rx) = MockRepoClient::new(MockMergeWorkFlow); - client.config.enabled = false; + client.config = Some(GitHubBrawlRepoConfig { + enabled: false, + ..Default::default() + }); tokio::spawn(async move { handle( diff --git a/server/src/command/retry.rs b/server/src/command/retry.rs index fd5ff9a..75c5b3e 100644 --- a/server/src/command/retry.rs +++ b/server/src/command/retry.rs @@ -23,7 +23,9 @@ async fn handle_with_pr( pr: PullRequest, context: BrawlCommandContext<'_, R>, ) -> anyhow::Result<()> { - if !context.repo.config().enabled { + let config = context.repo.config().await?; + + if !config.is_some_and(|c| c.enabled) { return Ok(()); } diff --git a/server/src/github/config.rs b/server/src/github/config.rs index ae034d9..89638a3 100644 --- a/server/src/github/config.rs +++ b/server/src/github/config.rs @@ -4,15 +4,13 @@ use std::str::FromStr; use serde::{Deserialize, Deserializer, Serialize, Serializer}; #[derive(Debug, Deserialize, Serialize, Clone, smart_default::SmartDefault)] -#[serde(default)] +#[serde(default, deny_unknown_fields)] pub struct GitHubBrawlRepoConfig { /// Whether Brawl is enabled for this repo #[default(true)] pub enabled: bool, /// Labels to attach to PRs on different states pub labels: GitHubBrawlLabelsConfig, - /// The target branches that this queue matches against. - pub branches: Vec, /// The branch prefix for @brawl try commands /// (default: "automation/brawl/try/") #[default("automation/brawl/try/")] @@ -59,13 +57,6 @@ pub struct GitHubBrawlRepoConfig { } impl GitHubBrawlRepoConfig { - pub fn missing() -> Self { - Self { - enabled: false, - ..Default::default() - } - } - pub fn try_permissions(&self) -> &[Permission] { self.try_permissions.as_ref().unwrap_or(&self.merge_permissions) } @@ -88,7 +79,7 @@ impl GitHubBrawlRepoConfig { } #[derive(Debug, Deserialize, Serialize, Clone, smart_default::SmartDefault)] -#[serde(default)] +#[serde(default, deny_unknown_fields)] pub struct GitHubBrawlLabelsConfig { /// The label to attach to PRs when they are in the merge queue #[serde(skip_serializing_if = "Vec::is_empty", deserialize_with = "string_or_vec")] @@ -314,19 +305,12 @@ mod tests { ); } - #[test] - fn test_missing_disabled() { - let config = GitHubBrawlRepoConfig::missing(); - assert!(!config.enabled); - } - #[test] fn test_default_config() { let config = GitHubBrawlRepoConfig::default(); let s = toml::to_string_pretty(&config).unwrap(); insta::assert_snapshot!(s, @r#" enabled = true - branches = [] try_branch_prefix = "automation/brawl/try/" merge_branch_prefix = "automation/brawl/merge/" temp_branch_prefix = "automation/brawl/temp/" @@ -343,10 +327,6 @@ mod tests { fn test_config_deserialize() { let config = r#" enabled = true - branches = [ - "main", - "staging", - ] try_branch_prefix = "automation/brawl/try/" merge_branch_prefix = "automation/brawl/merge/" temp_branch_prefix = "automation/brawl/temp/" @@ -366,7 +346,6 @@ mod tests { let config: GitHubBrawlRepoConfig = toml::from_str(config).unwrap(); assert!(config.enabled); - assert_eq!(config.branches, vec!["main", "staging"]); assert_eq!(config.try_branch_prefix, "automation/brawl/try/"); assert_eq!(config.merge_branch_prefix, "automation/brawl/merge/"); assert_eq!(config.temp_branch_prefix, "automation/brawl/temp/"); diff --git a/server/src/github/installation.rs b/server/src/github/installation.rs index e11e269..8684385 100644 --- a/server/src/github/installation.rs +++ b/server/src/github/installation.rs @@ -7,11 +7,12 @@ use anyhow::Context; use axum::http; use futures::{StreamExt, TryStreamExt}; use moka::future::Cache; +use octocrab::models::repos::Object; use octocrab::models::{InstallationRepositories, RepositoryId, UserId}; +use octocrab::params::repos::Reference; use octocrab::{GitHubError, Octocrab}; use parking_lot::Mutex; -use super::config::GitHubBrawlRepoConfig; use super::merge_workflow::DefaultMergeWorkflow; use super::models::{Installation, Repository, User}; use super::repo::{GitHubRepoClient, RepoClient, RepoClientRef}; @@ -25,18 +26,6 @@ pub struct InstallationClient { repo_lock: RepoLock, } -#[derive(Debug, thiserror::Error)] -pub enum GitHubBrawlRepoConfigError { - #[error("expected 1 file, got {0}")] - ExpectedOneFile(usize), - #[error("github error: {0}")] - GitHub(#[from] octocrab::Error), - #[error("toml error: {0}")] - Toml(#[from] toml::de::Error), - #[error("missing content")] - MissingContent, -} - #[derive(Debug, Clone)] pub struct OrgState { users: Cache>, @@ -82,60 +71,29 @@ impl InstallationClient { } } - async fn get_repo_config(&self, repo_id: RepositoryId) -> Result { - let file = match self - .client - .repos_by_id(repo_id) - .get_content() - .path(".github/brawl.toml") - .send() - .await - { - Ok(file) => file, - Err(octocrab::Error::GitHub { - source: - GitHubError { - status_code: http::StatusCode::NOT_FOUND, - .. - }, - .. - }) => { - return Ok(GitHubBrawlRepoConfig::missing()); + async fn set_repository(self: &Arc, repo: Repository) -> anyhow::Result<()> { + let base_commit_sha = if let Some(branch) = repo.default_branch.clone() { + match self.client.repos_by_id(repo.id).get_ref(&Reference::Branch(branch)).await?.object { + Object::Commit { sha, .. } => Some(sha), + Object::Tag { sha, .. } => Some(sha), + _ => anyhow::bail!("invalid object type for default branch"), } - Err(e) => return Err(e.into()), + } else { + None }; - if file.items.is_empty() { - return Ok(GitHubBrawlRepoConfig::missing()); - } - - if file.items.len() != 1 { - return Err(GitHubBrawlRepoConfigError::ExpectedOneFile(file.items.len())); - } - - let config = toml::from_str( - &file.items[0] - .decoded_content() - .ok_or(GitHubBrawlRepoConfigError::MissingContent)?, - )?; - - Ok(config) - } - - async fn set_repository(self: &Arc, repo: Repository) -> anyhow::Result<()> { - let config = self.get_repo_config(repo.id).await.context("get repo config")?; match self.repositories.lock().entry(repo.id) { Entry::Occupied(entry) => { - entry.get().config.store(Arc::new(config)); entry.get().repo.store(Arc::new(repo)); + entry.get().base_commit_sha.store(base_commit_sha.map(Arc::new)); } Entry::Vacant(entry) => { entry.insert(Arc::new(RepoClient::new( repo, - config, self.client.clone(), self.state.clone(), DefaultMergeWorkflow, + base_commit_sha, ))); } } @@ -337,64 +295,6 @@ mod tests { Arc::new(InstallationClient::new(client, installation)) } - #[tokio::test] - async fn test_get_repo_config() { - let (client, mut handle) = mock_octocrab(); - let installation_client = mock_installation_client(client); - - let task = tokio::spawn(async move { - assert!(installation_client.get_repo_config(RepositoryId(1)).await.unwrap().enabled); - assert!(!installation_client.get_repo_config(RepositoryId(2)).await.unwrap().enabled); - }); - - let (req, resp) = handle.next_request().await.unwrap(); - insta::assert_debug_snapshot!(debug_req(req).await, @r#" - DebugReq { - method: GET, - uri: "/repositories/1/contents/.github/brawl.toml?", - headers: [ - ( - "content-length", - "0", - ), - ( - "authorization", - "REDACTED", - ), - ], - body: None, - } - "#); - - resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_config.json"))); - - let (req, resp) = handle.next_request().await.unwrap(); - insta::assert_debug_snapshot!(debug_req(req).await, @r#" - DebugReq { - method: GET, - uri: "/repositories/2/contents/.github/brawl.toml?", - headers: [ - ( - "content-length", - "0", - ), - ( - "authorization", - "REDACTED", - ), - ], - body: None, - } - "#); - - resp.send_response(mock_response( - StatusCode::NOT_FOUND, - include_bytes!("mock/get_config_not_found.json"), - )); - - task.await.unwrap(); - } - #[tokio::test] async fn test_set_repository() { let (client, mut handle) = mock_octocrab(); @@ -410,7 +310,7 @@ mod tests { insta::assert_debug_snapshot!(debug_req(req).await, @r#" DebugReq { method: GET, - uri: "/repositories/0/contents/.github/brawl.toml?", + uri: "/repositories/0/git/ref/heads/main", headers: [ ( "content-length", @@ -425,13 +325,13 @@ mod tests { } "#); - resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_config.json"))); + resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_ref.json"))); let (req, resp) = handle.next_request().await.unwrap(); insta::assert_debug_snapshot!(debug_req(req).await, @r#" DebugReq { method: GET, - uri: "/repositories/0/contents/.github/brawl.toml?", + uri: "/repositories/0/git/ref/heads/main", headers: [ ( "content-length", @@ -446,7 +346,7 @@ mod tests { } "#); - resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_config.json"))); + resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_ref.json"))); let installation_client = task.await.unwrap(); assert!(installation_client.has_repository(RepositoryId(0))); @@ -490,7 +390,7 @@ mod tests { insta::assert_debug_snapshot!(debug_req(req).await, @r#" DebugReq { method: GET, - uri: "/repositories/1296269/contents/.github/brawl.toml?", + uri: "/repositories/1296269/git/ref/heads/master", headers: [ ( "content-length", @@ -505,7 +405,7 @@ mod tests { } "#); - resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_config.json"))); + resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_ref.json"))); let installation_client = task.await.unwrap(); assert_eq!(installation_client.repositories(), vec![RepositoryId(1296269)]); @@ -547,7 +447,7 @@ mod tests { insta::assert_debug_snapshot!(debug_req(req).await, @r#" DebugReq { method: GET, - uri: "/repositories/899726767/contents/.github/brawl.toml?", + uri: "/repositories/899726767/git/ref/heads/test/queue", headers: [ ( "content-length", @@ -562,7 +462,7 @@ mod tests { } "#); - resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_config.json"))); + resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_ref.json"))); let installation_client = task.await.unwrap(); assert!(installation_client.has_repository(RepositoryId(899726767))); @@ -603,7 +503,7 @@ mod tests { insta::assert_debug_snapshot!(debug_req(req).await, @r#" DebugReq { method: GET, - uri: "/repositories/0/contents/.github/brawl.toml?", + uri: "/repositories/0/git/ref/heads/main", headers: [ ( "content-length", @@ -618,7 +518,7 @@ mod tests { } "#); - resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_config.json"))); + resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_ref.json"))); let installation_client = task.await.unwrap(); assert!(installation_client.has_repository(RepositoryId(0))); diff --git a/server/src/github/label_state.rs b/server/src/github/label_state.rs index 4c76838..ce16cce 100644 --- a/server/src/github/label_state.rs +++ b/server/src/github/label_state.rs @@ -86,7 +86,9 @@ pub async fn update_labels( is_dry_run: bool, repo_client: &impl GitHubRepoClient, ) -> anyhow::Result<()> { - let config = repo_client.config(); + let Some(config) = repo_client.config().await? else { + return Ok(()); + }; let LabelsAdjustments { desired_labels, @@ -245,7 +247,7 @@ mod tests { let (mut repo_client, mut rx) = MockRepoClient::new(DefaultMergeWorkflow); - repo_client.config.labels = GitHubBrawlLabelsConfig { + repo_client.config.as_mut().unwrap().labels = GitHubBrawlLabelsConfig { on_merge_queued: vec!["queued".to_string()], on_try_in_progress: vec!["try".to_string(), "in_progress".to_string()], on_merge_in_progress: vec!["merge".to_string(), "in_progress".to_string()], @@ -384,7 +386,7 @@ mod tests { let (mut repo_client, mut rx) = MockRepoClient::new(DefaultMergeWorkflow); - repo_client.config.labels = GitHubBrawlLabelsConfig { + repo_client.config.as_mut().unwrap().labels = GitHubBrawlLabelsConfig { on_merge_queued: vec!["queued".to_string()], on_try_in_progress: vec!["try".to_string(), "in_progress".to_string()], on_merge_in_progress: vec!["merge".to_string(), "in_progress".to_string()], diff --git a/server/src/github/merge_workflow.rs b/server/src/github/merge_workflow.rs index d3a7a69..898d911 100644 --- a/server/src/github/merge_workflow.rs +++ b/server/src/github/merge_workflow.rs @@ -373,7 +373,9 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { let mut success = true; let mut required_checks = Vec::new(); let mut missing_checks = Vec::new(); - let config = repo.config(); + let Some(config) = repo.config().await? else { + return Ok(true); + }; for check in &config.required_status_checks { let Some(check) = checks.get(check.as_str()).copied() else { @@ -403,7 +405,7 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { if success { self.success(run, repo, conn, pr, required_checks.as_ref()).await?; } else if chrono::Utc::now().signed_duration_since(started_at) - > chrono::Duration::minutes(repo.config().timeout_minutes as i64) + > chrono::Duration::minutes(config.timeout_minutes as i64) { self.fail( run, @@ -411,7 +413,7 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { repo, conn, &messages::tests_timeout( - repo.config().timeout_minutes, + config.timeout_minutes, format_fn(|f| { for (name, check) in &missing_checks { if let Some(check) = check { @@ -438,6 +440,10 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { conn: &mut AsyncPgConnection, pr: &Pr<'_>, ) -> anyhow::Result { + let Some(config) = repo.config().await? else { + return Ok(false); + }; + let update = CiRun::update(run.id) .status(GithubCiRunStatus::InProgress) .started_at(chrono::Utc::now()); @@ -506,7 +512,7 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { }), ); - let commit = match repo.create_merge(&commit_message, &base_sha, &run.head_commit_sha).await { + let commit = match repo.create_merge(&commit_message, &base_sha, &run.head_commit_sha, &config).await { Ok(MergeResult::Success(commit)) => commit, Ok(MergeResult::Conflict) => { self.fail( @@ -892,7 +898,7 @@ mod tests { ) .await; - client.config.labels = GitHubBrawlLabelsConfig { + client.config.as_mut().unwrap().labels = GitHubBrawlLabelsConfig { on_merge_queued: vec!["merge_queued".to_string()], on_merge_in_progress: vec!["merge_in_progress".to_string()], on_merge_success: vec!["merge_success".to_string()], @@ -1332,7 +1338,7 @@ mod tests { ) .await; - client.config.labels = GitHubBrawlLabelsConfig { + client.config.as_mut().unwrap().labels = GitHubBrawlLabelsConfig { on_merge_queued: vec!["merge_queued".to_string()], on_merge_in_progress: vec!["merge_in_progress".to_string()], on_merge_success: vec!["merge_success".to_string()], @@ -1454,10 +1460,10 @@ mod tests { ) .await; - let client = client.with_config(GitHubBrawlRepoConfig { + let client = client.with_config(Some(GitHubBrawlRepoConfig { timeout_minutes: 0, ..Default::default() - }); + })); UpdateCiRun::builder(run.id) .status(GithubCiRunStatus::InProgress) @@ -1546,7 +1552,7 @@ mod tests { ) .await; - client.config.labels = GitHubBrawlLabelsConfig { + client.config.as_mut().unwrap().labels = GitHubBrawlLabelsConfig { on_merge_queued: vec!["merge_queued".to_string()], on_merge_in_progress: vec!["merge_in_progress".to_string()], on_merge_success: vec!["merge_success".to_string()], @@ -1603,6 +1609,7 @@ mod tests { message, head_sha, result, + .. } => { assert_eq!(base_sha, "sha"); assert_eq!(head_sha, "head_commit_sha"); @@ -1758,6 +1765,7 @@ mod tests { message, head_sha, result, + .. } => { assert_eq!(base_sha, "branch_commit_sha"); assert_eq!(head_sha, "head_commit_sha"); @@ -2192,6 +2200,7 @@ mod tests { message, head_sha, result, + .. } => { assert_eq!(base_sha, "sha"); assert_eq!(head_sha, "head_commit_sha"); @@ -2286,7 +2295,7 @@ mod tests { ) .await; - client.config.labels = GitHubBrawlLabelsConfig { + client.config.as_mut().unwrap().labels = GitHubBrawlLabelsConfig { on_merge_queued: vec!["merge_queued".to_string()], on_merge_in_progress: vec!["merge_in_progress".to_string()], on_merge_success: vec!["merge_success".to_string()], diff --git a/server/src/github/mock/get_ref.json b/server/src/github/mock/get_ref.json new file mode 100644 index 0000000..c10ed0c --- /dev/null +++ b/server/src/github/mock/get_ref.json @@ -0,0 +1,10 @@ +{ + "ref": "refs/heads/main", + "node_id": "REF_kwDONaC9r7VyZWZzL2hlYWRzL3Rlc3QvcXVldWU", + "url": "https://api.github.com/repos/ScuffleCloud/ci-testing/git/refs/heads/main", + "object": { + "sha": "b7f8cd1bd474d5be1802377c9a0baea5eb59fcb7", + "type": "commit", + "url": "https://api.github.com/repos/ScuffleCloud/ci-testing/git/commits/b7f8cd1bd474d5be1802377c9a0baea5eb59fcb7" + } +} diff --git a/server/src/github/mod.rs b/server/src/github/mod.rs index 2a287f9..a4ab70a 100644 --- a/server/src/github/mod.rs +++ b/server/src/github/mod.rs @@ -149,7 +149,6 @@ mod test_utils { use octocrab::models::{AppId, RepositoryId, UserId}; use octocrab::{AuthState, Octocrab, OctocrabBuilder}; - use super::config::GitHubBrawlRepoConfig; use super::installation::OrgState; use super::merge_workflow::GitHubMergeWorkflow; use super::models::{Repository, User}; @@ -231,10 +230,10 @@ mod test_utils { pub async fn mock_repo_client( octocrab: Octocrab, repo: Repository, - config: GitHubBrawlRepoConfig, merge_workflow: MockMergeWorkflow, + base_commit_sha: Option, ) -> RepoClient { - RepoClient::new(repo, config, octocrab, OrgState::default(), merge_workflow) + RepoClient::new(repo, octocrab, OrgState::default(), merge_workflow, base_commit_sha) } pub fn default_repo() -> Repository { @@ -245,6 +244,7 @@ mod test_utils { login: "ScuffleCloud".to_owned(), id: UserId(122814584), }, + default_branch: Some("main".to_string()), } } @@ -380,7 +380,7 @@ mod test { insta::assert_debug_snapshot!(debug_req(req).await, @r#" DebugReq { method: GET, - uri: "/repositories/1296269/contents/.github/brawl.toml?", + uri: "/repositories/1296269/git/ref/heads/master", headers: [ ( "content-length", @@ -395,7 +395,7 @@ mod test { } "#); - resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_config.json"))); + resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_ref.json"))); let service = service.await.unwrap(); @@ -461,7 +461,7 @@ mod test { insta::assert_debug_snapshot!(debug_req(req).await, @r#" DebugReq { method: GET, - uri: "/repositories/1296269/contents/.github/brawl.toml?", + uri: "/repositories/1296269/git/ref/heads/master", headers: [ ( "content-length", @@ -525,7 +525,7 @@ mod test { insta::assert_debug_snapshot!(debug_req(req).await, @r#" DebugReq { method: GET, - uri: "/repositories/1296269/contents/.github/brawl.toml?", + uri: "/repositories/1296269/git/ref/heads/master", headers: [ ( "content-length", diff --git a/server/src/github/models.rs b/server/src/github/models.rs index 95b5c41..074a3c0 100644 --- a/server/src/github/models.rs +++ b/server/src/github/models.rs @@ -207,6 +207,7 @@ pub struct Repository { pub id: RepositoryId, pub name: String, pub owner: User, + pub default_branch: Option, } impl From for Repository { @@ -215,6 +216,7 @@ impl From for Repository { id: value.id, name: value.name, owner: value.owner.expect("repository has no owner").into(), + default_branch: value.default_branch, } } } @@ -225,6 +227,7 @@ impl Default for Repository { id: RepositoryId(0), name: "repo".to_string(), owner: User::default(), + default_branch: Some("main".to_string()), } } } diff --git a/server/src/github/repo.rs b/server/src/github/repo.rs index 587ff95..486daec 100644 --- a/server/src/github/repo.rs +++ b/server/src/github/repo.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Context; -use arc_swap::ArcSwap; +use arc_swap::{ArcSwap, ArcSwapOption}; use axum::http; use moka::future::Cache; use octocrab::models::repos::{Object, Ref}; @@ -21,7 +21,8 @@ use super::models::{Commit, Label, PullRequest, Repository, Review, User, Workfl pub struct RepoClient { pub(super) repo: ArcSwap, - pub(super) config: ArcSwap, + pub(super) base_commit_sha: ArcSwapOption, + configs: Cache>>, client: Octocrab, org_state: OrgState, merge_workflow: W, @@ -74,7 +75,9 @@ where fn can_merge(&self, user_id: UserId) -> impl std::future::Future> + Send; fn can_try(&self, user_id: UserId) -> impl std::future::Future> + Send; fn can_review(&self, user_id: UserId) -> impl std::future::Future> + Send; - fn config(&self) -> Arc; + fn config(&self) -> impl std::future::Future>>> + Send; + fn config_at_commit(&self, sha: &str) -> impl std::future::Future>>> + Send; + fn base_commit_sha(&self) -> Option>; fn owner(&self) -> String; fn name(&self) -> String; fn merge_workflow(&self) -> Self::MergeWorkflow<'_>; @@ -90,7 +93,7 @@ where fn get_reviewers(&self, pr_number: u64) -> impl std::future::Future>> + Send; fn send_message(&self, issue_number: u64, message: &IssueMessage) -> impl std::future::Future> + Send; fn get_commit(&self, sha: &str) -> impl std::future::Future>> + Send; - fn create_merge(&self, message: &CommitMessage, base_sha: &str, head_sha: &str) -> impl std::future::Future> + Send; + fn create_merge(&self, message: &CommitMessage, base_sha: &str, head_sha: &str, config: &GitHubBrawlRepoConfig) -> impl std::future::Future> + Send; fn commit_link(&self, sha: &str) -> String; fn workflow_run_link(&self, run_id: RunId) -> String; fn pr_link(&self, pr_number: u64) -> String; @@ -111,9 +114,23 @@ pub trait GitHubRepoClient: Send + Sync { /// The ID of the repository fn id(&self) -> RepositoryId; - /// The repository configuration - fn config(&self) -> Arc; + /// The repository configuration (default branch) + fn config(&self) -> impl std::future::Future>>> + Send { + async move { + let Some(sha) = self.base_commit_sha() else { + return Ok(None); + }; + + self.config_at_commit(&sha).await + } + } + + fn config_at_commit(&self, sha: &str) -> impl std::future::Future>>> + Send; + + /// The base commit SHA + fn base_commit_sha(&self) -> Option>; + /// The owner of the repository /// The owner of the repository fn owner(&self) -> String; @@ -181,6 +198,7 @@ pub trait GitHubRepoClient: Send + Sync { message: &CommitMessage, base_sha: &str, head_sha: &str, + config: &GitHubBrawlRepoConfig, ) -> impl std::future::Future> + Send; /// Create a commit @@ -237,19 +255,34 @@ pub trait GitHubRepoClient: Send + Sync { /// Check if a user can merge fn can_merge(&self, user_id: UserId) -> impl std::future::Future> + Send { - async move { self.has_permission(user_id, &self.config().as_ref().merge_permissions).await } + async move { + let Some(config) = self.config().await? else { + return Ok(false); + }; + + self.has_permission(user_id, &config.merge_permissions).await + } } /// Check if a user can try fn can_try(&self, user_id: UserId) -> impl std::future::Future> + Send { - async move { self.has_permission(user_id, self.config().as_ref().try_permissions()).await } + async move { + let Some(config) = self.config().await? else { + return Ok(false); + }; + + self.has_permission(user_id, config.try_permissions()).await + } } /// Check if a user can review fn can_review(&self, user_id: UserId) -> impl std::future::Future> + Send { async move { - self.has_permission(user_id, self.config().as_ref().reviewer_permissions()) - .await + let Some(config) = self.config().await? else { + return Ok(false); + }; + + self.has_permission(user_id, config.reviewer_permissions()).await } } } @@ -257,10 +290,10 @@ pub trait GitHubRepoClient: Send + Sync { impl RepoClient { pub(super) fn new( repo: Repository, - config: GitHubBrawlRepoConfig, client: Octocrab, user_cache: OrgState, merge_workflow: W, + base_commit_sha: Option, ) -> Self { tracing::info!( id = %repo.id, @@ -271,7 +304,11 @@ impl RepoClient { Self { repo: ArcSwap::from_pointee(repo), - config: ArcSwap::from_pointee(config), + configs: Cache::builder() + .max_capacity(100) + .time_to_idle(Duration::from_secs(600)) + .time_to_live(Duration::from_secs(24 * 60 * 60)) + .build(), client, org_state: user_cache, merge_workflow, @@ -279,10 +316,24 @@ impl RepoClient { .max_capacity(50) .time_to_live(Duration::from_secs(60)) .build(), + base_commit_sha: ArcSwapOption::from_pointee(base_commit_sha), } } } +#[derive(Debug, thiserror::Error)] +pub enum GitHubBrawlRepoConfigError { + #[error("expected 1 file, got {0}")] + ExpectedOneFile(usize), + #[error("github error: {0}")] + GitHub(#[from] octocrab::Error), + #[error("toml error: {0}")] + Toml(#[from] toml::de::Error), + #[error("missing content")] + MissingContent, +} + + impl GitHubRepoClient for RepoClient { type MergeWorkflow<'a> = &'a W @@ -297,8 +348,53 @@ impl GitHubRepoClient for RepoClient { &self.merge_workflow } - fn config(&self) -> Arc { - self.config.load_full() + async fn config_at_commit(&self, sha: &str) -> anyhow::Result>> { + let config = self.configs.try_get_with_by_ref::<_, GitHubBrawlRepoConfigError, _>(sha, async { + let file = match self + .client + .repos_by_id(self.id()) + .get_content() + .path(".github/brawl.toml") + .r#ref(sha) + .send() + .await + { + Ok(file) => file, + Err(octocrab::Error::GitHub { + source: + GitHubError { + status_code: http::StatusCode::NOT_FOUND, + .. + }, + .. + }) => { + return Ok(None); + } + Err(e) => return Err(e.into()), + }; + + if file.items.is_empty() { + return Ok(None); + } + + if file.items.len() != 1 { + return Err(GitHubBrawlRepoConfigError::ExpectedOneFile(file.items.len())); + } + + let config = toml::from_str( + &file.items[0] + .decoded_content() + .ok_or(GitHubBrawlRepoConfigError::MissingContent)?, + )?; + + Ok(Some(Arc::new(config))) + }).await?; + + Ok(config) + } + + fn base_commit_sha(&self) -> Option> { + self.base_commit_sha.load_full() } fn name(&self) -> String { @@ -348,8 +444,8 @@ impl GitHubRepoClient for RepoClient { Ok(()) } - async fn create_merge(&self, message: &CommitMessage, base_sha: &str, head_sha: &str) -> anyhow::Result { - let tmp_branch = self.config().temp_branch(); + async fn create_merge(&self, message: &CommitMessage, base_sha: &str, head_sha: &str, config: &GitHubBrawlRepoConfig) -> anyhow::Result { + let tmp_branch = config.temp_branch(); self.push_branch(&tmp_branch, base_sha, true) .await @@ -604,7 +700,8 @@ pub mod test_utils { pub id: RepositoryId, pub owner: String, pub name: String, - pub config: GitHubBrawlRepoConfig, + pub config: Option, + pub base_commit_sha: Option, pub actions: tokio::sync::mpsc::Sender, pub merge_workflow: T, } @@ -617,7 +714,8 @@ pub mod test_utils { id: RepositoryId(1), owner: "owner".to_owned(), name: "repo".to_owned(), - config: GitHubBrawlRepoConfig::default(), + config: Some(GitHubBrawlRepoConfig::default()), + base_commit_sha: Some("base".to_owned()), actions: tx, merge_workflow, }, @@ -629,8 +727,12 @@ pub mod test_utils { Self { id, ..self } } - pub fn with_config(self, config: GitHubBrawlRepoConfig) -> Self { - Self { config, ..self } + pub fn with_config(self, config: impl Into>) -> Self { + Self { config: config.into(), ..self } + } + + pub fn with_base_commit_sha(self, base_commit_sha: impl Into>) -> Self { + Self { base_commit_sha: base_commit_sha.into(), ..self } } pub fn with_owner(self, owner: String) -> Self { @@ -676,6 +778,7 @@ pub mod test_utils { message: CommitMessage, base_sha: String, head_sha: String, + config: GitHubBrawlRepoConfig, result: tokio::sync::oneshot::Sender>, }, CreateCommit { @@ -721,6 +824,10 @@ pub mod test_utils { run_id: RunId, result: tokio::sync::oneshot::Sender>, }, + GetConfigAtCommit { + sha: String, + result: tokio::sync::oneshot::Sender>>>, + }, } impl GitHubRepoClient for MockRepoClient { @@ -737,8 +844,20 @@ pub mod test_utils { self.id } - fn config(&self) -> Arc { - Arc::new(self.config.clone()) + async fn config(&self) -> anyhow::Result>> { + let Some(config) = self.config.clone() else { + if let Some(base_commit_sha) = self.base_commit_sha.as_deref() { + return self.config_at_commit(base_commit_sha).await; + } + + return Ok(None); + }; + + Ok(Some(Arc::new(config))) + } + + fn base_commit_sha(&self) -> Option> { + self.base_commit_sha.clone().map(Arc::new) } fn owner(&self) -> String { @@ -815,6 +934,7 @@ pub mod test_utils { message: &CommitMessage, base_sha: &str, head_sha: &str, + config: &GitHubBrawlRepoConfig, ) -> anyhow::Result { let (tx, rx) = tokio::sync::oneshot::channel(); self.actions @@ -822,6 +942,7 @@ pub mod test_utils { message: message.clone(), base_sha: base_sha.to_string(), head_sha: head_sha.to_string(), + config: config.clone(), result: tx, }) .await @@ -940,6 +1061,15 @@ pub mod test_utils { .expect("send cancel workflow"); rx.await.expect("recv cancel workflow") } + + async fn config_at_commit(&self, sha: &str) -> anyhow::Result>> { + let (tx, rx) = tokio::sync::oneshot::channel(); + self.actions + .send(MockRepoAction::GetConfigAtCommit { sha: sha.to_string(), result: tx }) + .await + .expect("send get config at commit"); + rx.await.expect("recv get config at commit") + } } } @@ -960,15 +1090,15 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; assert_eq!(repo_client.id(), RepositoryId(899726767)); assert_eq!(repo_client.owner(), "ScuffleCloud"); assert_eq!(repo_client.name(), "ci-testing"); - assert!(repo_client.config().enabled); + assert_eq!(repo_client.base_commit_sha().unwrap().as_ref(), "base"); assert_eq!(repo_client.merge_workflow().0, 1); } @@ -978,8 +1108,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1027,8 +1157,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1071,8 +1201,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1220,8 +1350,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1267,8 +1397,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1310,8 +1440,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1369,8 +1499,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1434,8 +1564,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1530,8 +1660,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1694,8 +1824,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1734,8 +1864,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1848,8 +1978,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -1866,7 +1996,8 @@ mod tests { "TroyKomodo", ), "b7f8cd1bd474d5be1802377c9a0baea5eb59fcb7", - "b7f8cd1bd474d5be1802377c9a0baea5eb59fcb6" + "b7f8cd1bd474d5be1802377c9a0baea5eb59fcb6", + &GitHubBrawlRepoConfig::default() ) .await .unwrap(), @@ -1884,7 +2015,8 @@ mod tests { "TroyKomodo", ), "b7f8cd1bd474d5be1802377c9a0baea5eb59fcb7", - "conflicting-sha" + "conflicting-sha", + &GitHubBrawlRepoConfig::default() ) .await .unwrap(), @@ -2010,8 +2142,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -2056,8 +2188,8 @@ mod tests { let repo_client = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; @@ -2096,8 +2228,8 @@ mod tests { let repo = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; assert_eq!( @@ -2112,8 +2244,8 @@ mod tests { let repo = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; assert_eq!(repo.pr_link(1), "https://github.com/ScuffleCloud/ci-testing/pull/1"); @@ -2125,8 +2257,8 @@ mod tests { let repo = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; assert_eq!(repo.owner(), "ScuffleCloud"); @@ -2138,8 +2270,8 @@ mod tests { let repo = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; assert_eq!(repo.name(), "ci-testing"); @@ -2151,8 +2283,8 @@ mod tests { let repo = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; assert_eq!( @@ -2168,8 +2300,8 @@ mod tests { let repo = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; let task = tokio::spawn(async move { @@ -2205,8 +2337,8 @@ mod tests { let repo = mock_repo_client( octocrab, default_repo(), - GitHubBrawlRepoConfig::default(), MockMergeWorkflow(1), + Some("base".to_owned()), ) .await; let task = tokio::spawn(async move { @@ -2236,4 +2368,72 @@ mod tests { task.await.unwrap(); } + + #[tokio::test] + async fn test_repo_get_config() { + let (octocrab, mut handle) = mock_octocrab(); + let repo = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; + + let task = tokio::spawn(async move { + let config = repo.config().await.unwrap().unwrap(); + assert_eq!(config.enabled, true); + }); + + let (req, resp) = handle.next_request().await.unwrap(); + insta::assert_debug_snapshot!(debug_req(req).await, @r#" + DebugReq { + method: GET, + uri: "/repositories/899726767/contents/.github/brawl.toml?ref=base", + headers: [ + ( + "content-length", + "0", + ), + ( + "authorization", + "REDACTED", + ), + ], + body: None, + } + "#); + + resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_config.json"))); + + task.await.unwrap(); + } + + #[tokio::test] + async fn test_repo_get_config_at_ref() { + let (octocrab, mut handle) = mock_octocrab(); + let repo = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; + + let task = tokio::spawn(async move { + let config = repo.config_at_commit("b7f8cd1bd474d5be1802377c9a0baea5eb59fcb6").await.unwrap().unwrap(); + assert_eq!(config.enabled, true); + }); + + let (req, resp) = handle.next_request().await.unwrap(); + insta::assert_debug_snapshot!(debug_req(req).await, @r#" + DebugReq { + method: GET, + uri: "/repositories/899726767/contents/.github/brawl.toml?ref=b7f8cd1bd474d5be1802377c9a0baea5eb59fcb6", + headers: [ + ( + "content-length", + "0", + ), + ( + "authorization", + "REDACTED", + ), + ], + body: None, + } + "#); + + resp.send_response(mock_response(StatusCode::OK, include_bytes!("mock/get_config.json"))); + + task.await.unwrap(); + } } diff --git a/server/src/webhook/check_event.rs b/server/src/webhook/check_event.rs index b06581a..c03839d 100644 --- a/server/src/webhook/check_event.rs +++ b/server/src/webhook/check_event.rs @@ -31,8 +31,11 @@ pub async fn handle( return Ok(()); } - let required = repo - .config() + let Some(config) = repo.config().await? else { + return Ok(()); + }; + + let required = config .required_status_checks .iter() .any(|check| check.eq_ignore_ascii_case(&check_run_event.name)); From 6888cc6fe8319a5d8e0d6a17bd327a322c59db6f Mon Sep 17 00:00:00 2001 From: Troy Benson Date: Fri, 3 Jan 2025 22:29:47 +0000 Subject: [PATCH 2/2] fix formatting & lint --- server/src/command/dry_run.rs | 2 +- server/src/github/installation.rs | 8 +- server/src/github/merge_workflow.rs | 5 +- server/src/github/repo.rs | 298 +++++++++------------------- 4 files changed, 107 insertions(+), 206 deletions(-) diff --git a/server/src/command/dry_run.rs b/server/src/command/dry_run.rs index f6e69bd..53eec6a 100644 --- a/server/src/command/dry_run.rs +++ b/server/src/command/dry_run.rs @@ -924,7 +924,7 @@ mod tests { let pr = PullRequest { number: 1, - merged_at: Some(Utc::now().into()), + merged_at: Some(Utc::now()), ..Default::default() }; diff --git a/server/src/github/installation.rs b/server/src/github/installation.rs index 8684385..79b4300 100644 --- a/server/src/github/installation.rs +++ b/server/src/github/installation.rs @@ -73,7 +73,13 @@ impl InstallationClient { async fn set_repository(self: &Arc, repo: Repository) -> anyhow::Result<()> { let base_commit_sha = if let Some(branch) = repo.default_branch.clone() { - match self.client.repos_by_id(repo.id).get_ref(&Reference::Branch(branch)).await?.object { + match self + .client + .repos_by_id(repo.id) + .get_ref(&Reference::Branch(branch)) + .await? + .object + { Object::Commit { sha, .. } => Some(sha), Object::Tag { sha, .. } => Some(sha), _ => anyhow::bail!("invalid object type for default branch"), diff --git a/server/src/github/merge_workflow.rs b/server/src/github/merge_workflow.rs index 898d911..fadad41 100644 --- a/server/src/github/merge_workflow.rs +++ b/server/src/github/merge_workflow.rs @@ -512,7 +512,10 @@ impl GitHubMergeWorkflow for DefaultMergeWorkflow { }), ); - let commit = match repo.create_merge(&commit_message, &base_sha, &run.head_commit_sha, &config).await { + let commit = match repo + .create_merge(&commit_message, &base_sha, &run.head_commit_sha, &config) + .await + { Ok(MergeResult::Success(commit)) => commit, Ok(MergeResult::Conflict) => { self.fail( diff --git a/server/src/github/repo.rs b/server/src/github/repo.rs index 486daec..71cd424 100644 --- a/server/src/github/repo.rs +++ b/server/src/github/repo.rs @@ -125,7 +125,10 @@ pub trait GitHubRepoClient: Send + Sync { } } - fn config_at_commit(&self, sha: &str) -> impl std::future::Future>>> + Send; + fn config_at_commit( + &self, + sha: &str, + ) -> impl std::future::Future>>> + Send; /// The base commit SHA fn base_commit_sha(&self) -> Option>; @@ -333,7 +336,6 @@ pub enum GitHubBrawlRepoConfigError { MissingContent, } - impl GitHubRepoClient for RepoClient { type MergeWorkflow<'a> = &'a W @@ -349,46 +351,49 @@ impl GitHubRepoClient for RepoClient { } async fn config_at_commit(&self, sha: &str) -> anyhow::Result>> { - let config = self.configs.try_get_with_by_ref::<_, GitHubBrawlRepoConfigError, _>(sha, async { - let file = match self - .client - .repos_by_id(self.id()) - .get_content() - .path(".github/brawl.toml") - .r#ref(sha) - .send() - .await - { - Ok(file) => file, - Err(octocrab::Error::GitHub { - source: - GitHubError { - status_code: http::StatusCode::NOT_FOUND, - .. - }, - .. - }) => { + let config = self + .configs + .try_get_with_by_ref::<_, GitHubBrawlRepoConfigError, _>(sha, async { + let file = match self + .client + .repos_by_id(self.id()) + .get_content() + .path(".github/brawl.toml") + .r#ref(sha) + .send() + .await + { + Ok(file) => file, + Err(octocrab::Error::GitHub { + source: + GitHubError { + status_code: http::StatusCode::NOT_FOUND, + .. + }, + .. + }) => { + return Ok(None); + } + Err(e) => return Err(e.into()), + }; + + if file.items.is_empty() { return Ok(None); } - Err(e) => return Err(e.into()), - }; - - if file.items.is_empty() { - return Ok(None); - } - - if file.items.len() != 1 { - return Err(GitHubBrawlRepoConfigError::ExpectedOneFile(file.items.len())); - } - - let config = toml::from_str( - &file.items[0] - .decoded_content() - .ok_or(GitHubBrawlRepoConfigError::MissingContent)?, - )?; - - Ok(Some(Arc::new(config))) - }).await?; + + if file.items.len() != 1 { + return Err(GitHubBrawlRepoConfigError::ExpectedOneFile(file.items.len())); + } + + let config = toml::from_str( + &file.items[0] + .decoded_content() + .ok_or(GitHubBrawlRepoConfigError::MissingContent)?, + )?; + + Ok(Some(Arc::new(config))) + }) + .await?; Ok(config) } @@ -444,7 +449,13 @@ impl GitHubRepoClient for RepoClient { Ok(()) } - async fn create_merge(&self, message: &CommitMessage, base_sha: &str, head_sha: &str, config: &GitHubBrawlRepoConfig) -> anyhow::Result { + async fn create_merge( + &self, + message: &CommitMessage, + base_sha: &str, + head_sha: &str, + config: &GitHubBrawlRepoConfig, + ) -> anyhow::Result { let tmp_branch = config.temp_branch(); self.push_branch(&tmp_branch, base_sha, true) @@ -728,11 +739,17 @@ pub mod test_utils { } pub fn with_config(self, config: impl Into>) -> Self { - Self { config: config.into(), ..self } + Self { + config: config.into(), + ..self + } } pub fn with_base_commit_sha(self, base_commit_sha: impl Into>) -> Self { - Self { base_commit_sha: base_commit_sha.into(), ..self } + Self { + base_commit_sha: base_commit_sha.into(), + ..self + } } pub fn with_owner(self, owner: String) -> Self { @@ -778,7 +795,7 @@ pub mod test_utils { message: CommitMessage, base_sha: String, head_sha: String, - config: GitHubBrawlRepoConfig, + config: Box, result: tokio::sync::oneshot::Sender>, }, CreateCommit { @@ -942,7 +959,7 @@ pub mod test_utils { message: message.clone(), base_sha: base_sha.to_string(), head_sha: head_sha.to_string(), - config: config.clone(), + config: Box::new(config.clone()), result: tx, }) .await @@ -1065,7 +1082,10 @@ pub mod test_utils { async fn config_at_commit(&self, sha: &str) -> anyhow::Result>> { let (tx, rx) = tokio::sync::oneshot::channel(); self.actions - .send(MockRepoAction::GetConfigAtCommit { sha: sha.to_string(), result: tx }) + .send(MockRepoAction::GetConfigAtCommit { + sha: sha.to_string(), + result: tx, + }) .await .expect("send get config at commit"); rx.await.expect("recv get config at commit") @@ -1087,13 +1107,7 @@ mod tests { #[tokio::test] async fn test_repo_client_accessors() { let (octocrab, _) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; assert_eq!(repo_client.id(), RepositoryId(899726767)); assert_eq!(repo_client.owner(), "ScuffleCloud"); @@ -1105,13 +1119,7 @@ mod tests { #[tokio::test] async fn test_repo_client_get_user() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { let user = repo_client.get_user(UserId(49777269)).await.unwrap().unwrap(); @@ -1154,13 +1162,7 @@ mod tests { #[tokio::test] async fn test_repo_client_get_pull_request() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { let pull_request = repo_client.get_pull_request(22).await.unwrap(); @@ -1198,13 +1200,7 @@ mod tests { #[tokio::test] async fn test_repo_client_get_role_members() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { let members = repo_client.get_role_members(Role::Admin).await.unwrap(); @@ -1347,13 +1343,7 @@ mod tests { #[tokio::test] async fn test_repo_client_send_message() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo_client @@ -1394,13 +1384,7 @@ mod tests { #[tokio::test] async fn test_repo_client_get_commit() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo_client @@ -1437,13 +1421,7 @@ mod tests { #[tokio::test] async fn test_repo_client_create_commit() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo_client @@ -1496,13 +1474,7 @@ mod tests { #[tokio::test] async fn test_repo_client_delete_branch() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo_client.delete_branch("feature/1").await.unwrap(); @@ -1561,13 +1533,7 @@ mod tests { #[tokio::test] async fn test_repo_client_get_ref_latest_commit() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo_client @@ -1657,13 +1623,7 @@ mod tests { #[tokio::test] async fn test_repo_client_has_permission() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { assert!(repo_client @@ -1821,13 +1781,7 @@ mod tests { #[tokio::test] async fn test_repo_client_get_reviewers() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo_client.get_reviewers(22).await.unwrap(); @@ -1861,13 +1815,7 @@ mod tests { #[tokio::test] async fn test_repo_client_push_branch() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo_client @@ -1975,13 +1923,7 @@ mod tests { #[tokio::test] async fn test_repo_client_create_merge() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { assert!(matches!( @@ -2139,13 +2081,7 @@ mod tests { #[tokio::test] async fn test_repo_client_add_labels() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo_client.add_labels(1, &["queued".to_string()]).await.unwrap(); @@ -2185,13 +2121,7 @@ mod tests { #[tokio::test] async fn test_repo_client_remove_labels() { let (octocrab, mut handle) = mock_octocrab(); - let repo_client = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo_client = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo_client.remove_label(1, "some_label").await.unwrap(); @@ -2225,13 +2155,7 @@ mod tests { #[tokio::test] async fn test_repo_client_workflow_run_link() { let octocrab = octocrab::Octocrab::builder().personal_token("token").build().unwrap(); - let repo = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; assert_eq!( repo.workflow_run_link(RunId(1)), "https://github.com/ScuffleCloud/ci-testing/actions/runs/1" @@ -2241,52 +2165,28 @@ mod tests { #[tokio::test] async fn test_repo_client_pr_link() { let octocrab = octocrab::Octocrab::builder().personal_token("token").build().unwrap(); - let repo = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; assert_eq!(repo.pr_link(1), "https://github.com/ScuffleCloud/ci-testing/pull/1"); } #[tokio::test] async fn test_repo_client_owner() { let octocrab = octocrab::Octocrab::builder().personal_token("token").build().unwrap(); - let repo = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; assert_eq!(repo.owner(), "ScuffleCloud"); } #[tokio::test] async fn test_repo_client_name() { let octocrab = octocrab::Octocrab::builder().personal_token("token").build().unwrap(); - let repo = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; assert_eq!(repo.name(), "ci-testing"); } #[tokio::test] async fn test_repo_client_commit_link() { let octocrab = octocrab::Octocrab::builder().personal_token("token").build().unwrap(); - let repo = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; assert_eq!( repo.commit_link("b7f8cd1bd474d5be1802377c9a0baea5eb59fcb6"), "https://github.com/ScuffleCloud/ci-testing/commit/b7f8cd1bd474d5be1802377c9a0baea5eb59fcb6" @@ -2297,13 +2197,7 @@ mod tests { async fn test_repo_client_get_workflow_runs() { let (octocrab, mut handle) = mock_octocrab(); - let repo = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo.branch_workflows("main").await.unwrap(); }); @@ -2334,13 +2228,7 @@ mod tests { #[tokio::test] async fn test_repo_client_cancel_workflow_run() { let (octocrab, mut handle) = mock_octocrab(); - let repo = mock_repo_client( - octocrab, - default_repo(), - MockMergeWorkflow(1), - Some("base".to_owned()), - ) - .await; + let repo = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { repo.cancel_workflow_run(RunId(1)).await.unwrap(); }); @@ -2376,7 +2264,7 @@ mod tests { let task = tokio::spawn(async move { let config = repo.config().await.unwrap().unwrap(); - assert_eq!(config.enabled, true); + assert!(config.enabled); }); let (req, resp) = handle.next_request().await.unwrap(); @@ -2409,8 +2297,12 @@ mod tests { let repo = mock_repo_client(octocrab, default_repo(), MockMergeWorkflow(1), Some("base".to_owned())).await; let task = tokio::spawn(async move { - let config = repo.config_at_commit("b7f8cd1bd474d5be1802377c9a0baea5eb59fcb6").await.unwrap().unwrap(); - assert_eq!(config.enabled, true); + let config = repo + .config_at_commit("b7f8cd1bd474d5be1802377c9a0baea5eb59fcb6") + .await + .unwrap() + .unwrap(); + assert!(config.enabled); }); let (req, resp) = handle.next_request().await.unwrap();