From 55e1df4f583ff47efcd23345779b74aa1fc8ec37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 17 Mar 2024 16:28:42 +0100 Subject: [PATCH 1/9] Do not treat GitHub apps as bots --- src/github/mod.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/github/mod.rs b/src/github/mod.rs index dcb9670..0e05ed2 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -384,10 +384,12 @@ fn calculate_permission_diffs( permissions.push(diff); } // Bot permissions - let bots = expected_repo.bots.iter().map(|b| { - let bot_name = bot_name(b); + let bots = expected_repo.bots.iter().filter_map(|b| { + let Some(bot_name) = bot_name(b) else { + return None; + }; actual_teams.remove(bot_name); - (bot_name, RepoPermission::Write) + Some((bot_name, RepoPermission::Write)) }); // Member permissions let members = expected_repo @@ -441,13 +443,14 @@ fn calculate_permission_diffs( Ok(permissions) } -fn bot_name(bot: &Bot) -> &str { +/// Returns `None` if the bot is not an actual bot user, but rather a GitHub app. +fn bot_name(bot: &Bot) -> Option<&str> { match bot { - Bot::Bors => "bors", - Bot::Highfive => "rust-highfive", - Bot::RustTimer => "rust-timer", - Bot::Rustbot => "rustbot", - Bot::Rfcbot => "rfcbot", + Bot::Bors => Some("bors"), + Bot::Highfive => Some("rust-highfive"), + Bot::RustTimer => Some("rust-timer"), + Bot::Rustbot => Some("rustbot"), + Bot::Rfcbot => Some("rfcbot"), } } From ab81bd42a37cb523c7ac83f5bd0688e4dea5afff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 17 Mar 2024 15:35:45 +0100 Subject: [PATCH 2/9] Download information about organization app installations --- src/github/api/mod.rs | 16 ++++++++++++- src/github/api/read.rs | 51 ++++++++++++++++++++++++++++++++++++++++- src/github/mod.rs | 52 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 115 insertions(+), 4 deletions(-) diff --git a/src/github/api/mod.rs b/src/github/api/mod.rs index b049572..7ec3198 100644 --- a/src/github/api/mod.rs +++ b/src/github/api/mod.rs @@ -113,7 +113,7 @@ impl HttpClient { fn rest_paginated(&self, method: &Method, url: String, mut f: F) -> anyhow::Result<()> where - F: FnMut(Vec) -> anyhow::Result<()>, + F: FnMut(T) -> anyhow::Result<()>, T: DeserializeOwned, { let mut next = Some(url); @@ -248,10 +248,24 @@ impl fmt::Display for RepoPermission { } } +#[derive(serde::Deserialize, Debug)] +pub(crate) struct OrgAppInstallation { + #[serde(rename = "id")] + pub(crate) installation_id: u64, + pub(crate) app_id: u64, +} + +#[derive(serde::Deserialize, Debug)] +pub(crate) struct RepoAppInstallation { + pub(crate) name: String, +} + #[derive(serde::Deserialize, Debug)] pub(crate) struct Repo { #[serde(rename = "node_id")] pub(crate) id: String, + #[serde(rename = "id")] + pub(crate) repo_id: u64, pub(crate) name: String, #[serde(alias = "owner", deserialize_with = "repo_owner")] pub(crate) org: String, diff --git a/src/github/api/read.rs b/src/github/api/read.rs index ddf862f..5ace758 100644 --- a/src/github/api/read.rs +++ b/src/github/api/read.rs @@ -1,6 +1,7 @@ use crate::github::api::{ team_node_id, user_node_id, BranchProtection, GraphNode, GraphNodes, GraphPageInfo, HttpClient, - Login, Repo, RepoTeam, RepoUser, Team, TeamMember, TeamRole, + Login, OrgAppInstallation, Repo, RepoAppInstallation, RepoTeam, RepoUser, Team, TeamMember, + TeamRole, }; use reqwest::Method; use std::collections::{HashMap, HashSet}; @@ -12,6 +13,15 @@ pub(crate) trait GithubRead { /// Get the owners of an org fn org_owners(&self, org: &str) -> anyhow::Result>; + /// Get the app installations of an org + fn org_app_installations(&self, org: &str) -> anyhow::Result>; + + /// Get the repositories enabled for an app installation. + fn app_installation_repos( + &self, + installation_id: u64, + ) -> anyhow::Result>; + /// Get all teams associated with a org /// /// Returns a list of tuples of team name and slug @@ -110,6 +120,45 @@ impl GithubRead for GitHubApiRead { Ok(owners) } + fn org_app_installations(&self, org: &str) -> anyhow::Result> { + #[derive(serde::Deserialize, Debug)] + struct InstallationPage { + installations: Vec, + } + + let mut installations = Vec::new(); + self.client.rest_paginated( + &Method::GET, + format!("orgs/{org}/installations"), + |response: InstallationPage| { + installations.extend(response.installations); + Ok(()) + }, + )?; + Ok(installations) + } + + fn app_installation_repos( + &self, + installation_id: u64, + ) -> anyhow::Result> { + #[derive(serde::Deserialize, Debug)] + struct InstallationPage { + repositories: Vec, + } + + let mut installations = Vec::new(); + self.client.rest_paginated( + &Method::GET, + format!("user/installations/{installation_id}/repositories"), + |response: InstallationPage| { + installations.extend(response.repositories); + Ok(()) + }, + )?; + Ok(installations) + } + fn org_teams(&self, org: &str) -> anyhow::Result> { let mut teams = Vec::new(); diff --git a/src/github/mod.rs b/src/github/mod.rs index 0e05ed2..c5118b5 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -23,12 +23,37 @@ pub(crate) fn create_diff( github.diff_all() } +type OrgName = String; +type RepoName = String; + +#[derive(Clone, Debug)] +enum GithubApp { + RenovateBot, +} + +impl GithubApp { + fn from_id(app_id: u64) -> Option { + match app_id { + 2740 => Some(GithubApp::RenovateBot), + _ => None, + } + } +} + +#[derive(Clone, Debug)] +struct GithubAppInstallation { + app: GithubApp, + installation_id: u64, + repositories: HashSet, +} + struct SyncGitHub { github: Box, teams: Vec, repos: Vec, usernames_cache: HashMap, - org_owners: HashMap>, + org_owners: HashMap>, + org_apps: HashMap>, } impl SyncGitHub { @@ -50,15 +75,37 @@ impl SyncGitHub { let usernames_cache = github.usernames(&users)?; debug!("caching organization owners"); - let orgs = teams + let mut orgs = teams .iter() .filter_map(|t| t.github.as_ref()) .flat_map(|gh| &gh.teams) .map(|gh_team| &gh_team.org) .collect::>(); + let mut org_owners = HashMap::new(); + let mut org_apps = HashMap::new(); + for org in &orgs { org_owners.insert((*org).to_string(), github.org_owners(org)?); + + let mut installations: Vec = vec![]; + + for installation in github.org_app_installations(org)? { + if let Some(app) = GithubApp::from_id(installation.app_id) { + let mut repositories = HashSet::new(); + for repo_installation in + github.app_installation_repos(installation.installation_id)? + { + repositories.insert(repo_installation.name); + } + installations.push(GithubAppInstallation { + app, + installation_id: installation.installation_id, + repositories, + }); + } + } + org_apps.insert(org.to_string(), installations); } Ok(SyncGitHub { @@ -67,6 +114,7 @@ impl SyncGitHub { repos, usernames_cache, org_owners, + org_apps, }) } From cf9a3b9cdbacdcfd2808a728ed069968ae55b697 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 17 Mar 2024 16:12:06 +0100 Subject: [PATCH 3/9] Improve error messages for paginated REST requests --- src/github/api/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/github/api/mod.rs b/src/github/api/mod.rs index 7ec3198..61f3c61 100644 --- a/src/github/api/mod.rs +++ b/src/github/api/mod.rs @@ -138,7 +138,7 @@ impl HttpClient { } } - f(resp.json().with_context(|| { + f(resp.json_annotated().with_context(|| { format!("Failed to deserialize response body for {method} request to '{next_url}'") })?)?; } From 01ebd915bbd2e718420a462ded96a84eaf4a626e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 18 Mar 2024 14:15:14 +0100 Subject: [PATCH 4/9] Update `rust_team_data` --- Cargo.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index f65a464..ef9b6c5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -909,7 +909,7 @@ dependencies = [ [[package]] name = "rust_team_data" version = "1.0.0" -source = "git+https://github.com/rust-lang/team#cb7f3bd728847b4e59f02898fe7bbcb37b4ffdbb" +source = "git+https://github.com/rust-lang/team#c21e50b35eeb3a35e26001c79031537f224d5f2b" dependencies = [ "chacha20poly1305", "getrandom", From 78534b36ec5c601f4dc88289e73b80058959bd51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 17 Mar 2024 16:17:14 +0100 Subject: [PATCH 5/9] Install apps for new repositories --- src/github/api/write.rs | 18 ++++++ src/github/mod.rs | 120 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 131 insertions(+), 7 deletions(-) diff --git a/src/github/api/write.rs b/src/github/api/write.rs index 045a821..c93fd06 100644 --- a/src/github/api/write.rs +++ b/src/github/api/write.rs @@ -231,6 +231,7 @@ impl GitHubWrite { if self.dry_run { Ok(Repo { id: String::from("ID"), + repo_id: 0, name: name.to_string(), org: org.to_string(), description: settings.description.clone(), @@ -273,6 +274,23 @@ impl GitHubWrite { Ok(()) } + pub(crate) fn add_repo_to_app_installation( + &self, + installation_id: u64, + repository_id: u64, + ) -> anyhow::Result<()> { + debug!("Adding installation {installation_id} to repository {repository_id}"); + if !self.dry_run { + self.client + .req( + Method::PUT, + &format!("user/installations/{installation_id}/repositories/{repository_id}"), + )? + .send()?; + } + Ok(()) + } + /// Update a team's permissions to a repo pub(crate) fn update_team_repo_permissions( &self, diff --git a/src/github/mod.rs b/src/github/mod.rs index c5118b5..a1e8ed8 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -3,11 +3,13 @@ mod api; mod tests; use self::api::{BranchProtectionOp, TeamPrivacy, TeamRole}; -use crate::github::api::{GithubRead, Login, PushAllowanceActor, RepoPermission, RepoSettings}; +use crate::github::api::{ + GithubRead, Login, PushAllowanceActor, Repo, RepoPermission, RepoSettings, +}; use log::debug; use rust_team_data::v1::Bot; use std::collections::{HashMap, HashSet}; -use std::fmt::Write; +use std::fmt::{Display, Formatter, Write}; pub(crate) use self::api::{GitHubApiRead, GitHubWrite, HttpClient}; @@ -26,7 +28,7 @@ pub(crate) fn create_diff( type OrgName = String; type RepoName = String; -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq)] enum GithubApp { RenovateBot, } @@ -40,20 +42,34 @@ impl GithubApp { } } +impl Display for GithubApp { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + GithubApp::RenovateBot => f.write_str("RenovateBot"), + } + } +} + #[derive(Clone, Debug)] -struct GithubAppInstallation { +struct OrgAppInstallation { app: GithubApp, installation_id: u64, repositories: HashSet, } +#[derive(Clone, Debug, PartialEq)] +struct AppInstallation { + app: GithubApp, + installation_id: u64, +} + struct SyncGitHub { github: Box, teams: Vec, repos: Vec, usernames_cache: HashMap, org_owners: HashMap>, - org_apps: HashMap>, + org_apps: HashMap>, } impl SyncGitHub { @@ -88,7 +104,7 @@ impl SyncGitHub { for org in &orgs { org_owners.insert((*org).to_string(), github.org_owners(org)?); - let mut installations: Vec = vec![]; + let mut installations: Vec = vec![]; for installation in github.org_app_installations(org)? { if let Some(app) = GithubApp::from_id(installation.app_id) { @@ -98,7 +114,7 @@ impl SyncGitHub { { repositories.insert(repo_installation.name); } - installations.push(GithubAppInstallation { + installations.push(OrgAppInstallation { app, installation_id: installation.installation_id, repositories, @@ -299,6 +315,7 @@ impl SyncGitHub { }, permissions, branch_protections, + app_installations: self.diff_app_installations(expected_repo, &[])?, })); } }; @@ -392,6 +409,52 @@ impl SyncGitHub { Ok(branch_protection_diffs) } + fn diff_app_installations( + &self, + expected_repo: &rust_team_data::v1::Repo, + existing_installations: &[AppInstallation], + ) -> anyhow::Result> { + let mut diff = vec![]; + let mut found_apps = Vec::new(); + + // Find apps that should be enabled on the repository + for app in expected_repo.bots.iter().filter_map(|bot| match bot { + Bot::Renovate => Some(GithubApp::RenovateBot), + _ => None, + }) { + // Find installation ID of this app on GitHub + let gh_installation = self + .org_apps + .get(&expected_repo.org) + .and_then(|installations| { + installations + .iter() + .find(|installation| installation.app == app) + .map(|i| i.installation_id) + }); + let Some(gh_installation) = gh_installation else { + log::warn!("Application {app} should be enabled for repository {}/{}, but it is not installed on GitHub", expected_repo.org, expected_repo.name); + continue; + }; + let installation = AppInstallation { + app, + installation_id: gh_installation, + }; + found_apps.push(installation.clone()); + + if !existing_installations.contains(&installation) { + diff.push(AppInstallationDiff::Add(installation)); + } + } + for existing in existing_installations { + if !found_apps.contains(existing) { + diff.push(AppInstallationDiff::Remove(existing.clone())); + } + } + + Ok(diff) + } + fn expected_role(&self, org: &str, user: u64) -> TeamRole { if let Some(true) = self .org_owners @@ -499,6 +562,7 @@ fn bot_name(bot: &Bot) -> Option<&str> { Bot::RustTimer => Some("rust-timer"), Bot::Rustbot => Some("rustbot"), Bot::Rfcbot => Some("rfcbot"), + Bot::Renovate => None, } } @@ -619,6 +683,7 @@ struct CreateRepoDiff { settings: RepoSettings, permissions: Vec, branch_protections: Vec<(String, api::BranchProtection)>, + app_installations: Vec, } impl CreateRepoDiff { @@ -636,6 +701,11 @@ impl CreateRepoDiff { } .apply(sync, &self.org, &self.name, &repo.id)?; } + + for installation in &self.app_installations { + installation.apply(sync, &repo)?; + } + Ok(()) } } @@ -664,6 +734,10 @@ impl std::fmt::Display for CreateRepoDiff { writeln!(&mut f, " {branch_name}")?; log_branch_protection(branch_protection, None, &mut f)?; } + writeln!(f, " App Installations:")?; + for diff in &self.app_installations { + write!(f, "{diff}")?; + } Ok(()) } } @@ -930,6 +1004,38 @@ enum BranchProtectionDiffOperation { Delete(String), } +enum AppInstallationDiff { + Add(AppInstallation), + Remove(AppInstallation), +} + +impl AppInstallationDiff { + fn apply(&self, sync: &GitHubWrite, repo: &Repo) -> anyhow::Result<()> { + match self { + AppInstallationDiff::Add(app) => { + sync.add_repo_to_app_installation(app.installation_id, repo.repo_id)?; + } + AppInstallationDiff::Remove(_) => { + todo!() + } + } + Ok(()) + } +} + +impl std::fmt::Display for AppInstallationDiff { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + AppInstallationDiff::Add(app) => { + writeln!(f, " Install app {}", app.app) + } + AppInstallationDiff::Remove(app) => { + writeln!(f, " Remove app {}", app.app) + } + } + } +} + #[derive(Debug)] enum TeamDiff { Create(CreateTeamDiff), From ac6d6975a046743eb5beb583066189c9eb136581 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 17 Mar 2024 16:22:08 +0100 Subject: [PATCH 6/9] Rename `repo_id` to `repo_node_id` --- src/github/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/github/mod.rs b/src/github/mod.rs index a1e8ed8..b1ae33d 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -337,7 +337,7 @@ impl SyncGitHub { Ok(RepoDiff::Update(UpdateRepoDiff { org: expected_repo.org.clone(), name: actual_repo.name, - repo_id: actual_repo.id, + repo_node_id: actual_repo.id, settings_diff: (old_settings, new_settings), permission_diffs, branch_protection_diffs, @@ -745,7 +745,7 @@ impl std::fmt::Display for CreateRepoDiff { struct UpdateRepoDiff { org: String, name: String, - repo_id: String, + repo_node_id: String, // old, new settings_diff: (RepoSettings, RepoSettings), permission_diffs: Vec, @@ -786,7 +786,7 @@ impl UpdateRepoDiff { } for branch_protection in &self.branch_protection_diffs { - branch_protection.apply(sync, &self.org, &self.name, &self.repo_id)?; + branch_protection.apply(sync, &self.org, &self.name, &self.repo_node_id)?; } Ok(()) } From c13a6936887d72e819cb68be82abb9cc3fd0e0eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 17 Mar 2024 16:24:11 +0100 Subject: [PATCH 7/9] Rename `id` to `node_id` --- src/github/api/mod.rs | 3 +-- src/github/api/write.rs | 2 +- src/github/mod.rs | 4 ++-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/github/api/mod.rs b/src/github/api/mod.rs index 61f3c61..bf85349 100644 --- a/src/github/api/mod.rs +++ b/src/github/api/mod.rs @@ -262,8 +262,7 @@ pub(crate) struct RepoAppInstallation { #[derive(serde::Deserialize, Debug)] pub(crate) struct Repo { - #[serde(rename = "node_id")] - pub(crate) id: String, + pub(crate) node_id: String, #[serde(rename = "id")] pub(crate) repo_id: u64, pub(crate) name: String, diff --git a/src/github/api/write.rs b/src/github/api/write.rs index c93fd06..03e9090 100644 --- a/src/github/api/write.rs +++ b/src/github/api/write.rs @@ -230,7 +230,7 @@ impl GitHubWrite { debug!("Creating the repo {org}/{name} with {req:?}"); if self.dry_run { Ok(Repo { - id: String::from("ID"), + node_id: String::from("ID"), repo_id: 0, name: name.to_string(), org: org.to_string(), diff --git a/src/github/mod.rs b/src/github/mod.rs index b1ae33d..547a496 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -337,7 +337,7 @@ impl SyncGitHub { Ok(RepoDiff::Update(UpdateRepoDiff { org: expected_repo.org.clone(), name: actual_repo.name, - repo_node_id: actual_repo.id, + repo_node_id: actual_repo.node_id, settings_diff: (old_settings, new_settings), permission_diffs, branch_protection_diffs, @@ -699,7 +699,7 @@ impl CreateRepoDiff { pattern: branch.clone(), operation: BranchProtectionDiffOperation::Create(protection.clone()), } - .apply(sync, &self.org, &self.name, &repo.id)?; + .apply(sync, &self.org, &self.name, &repo.node_id)?; } for installation in &self.app_installations { From 6f067c41684089d44382a704fa6b53af89df4574 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 17 Mar 2024 16:26:46 +0100 Subject: [PATCH 8/9] Add removal of repositories from installations --- src/github/api/write.rs | 23 +++++++++++++++-- src/github/mod.rs | 55 +++++++++++++++++++++++++++++++++-------- 2 files changed, 66 insertions(+), 12 deletions(-) diff --git a/src/github/api/write.rs b/src/github/api/write.rs index 03e9090..1fdd5e9 100644 --- a/src/github/api/write.rs +++ b/src/github/api/write.rs @@ -279,14 +279,33 @@ impl GitHubWrite { installation_id: u64, repository_id: u64, ) -> anyhow::Result<()> { - debug!("Adding installation {installation_id} to repository {repository_id}"); + debug!("Adding repository {repository_id} to installation {installation_id}"); if !self.dry_run { self.client .req( Method::PUT, &format!("user/installations/{installation_id}/repositories/{repository_id}"), )? - .send()?; + .send()? + .custom_error_for_status()?; + } + Ok(()) + } + + pub(crate) fn remove_repo_from_app_installation( + &self, + installation_id: u64, + repository_id: u64, + ) -> anyhow::Result<()> { + debug!("Removing repository {repository_id} from installation {installation_id}"); + if !self.dry_run { + self.client + .req( + Method::DELETE, + &format!("user/installations/{installation_id}/repositories/{repository_id}"), + )? + .send()? + .custom_error_for_status()?; } Ok(()) } diff --git a/src/github/mod.rs b/src/github/mod.rs index 547a496..2ced5f5 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -3,9 +3,7 @@ mod api; mod tests; use self::api::{BranchProtectionOp, TeamPrivacy, TeamRole}; -use crate::github::api::{ - GithubRead, Login, PushAllowanceActor, Repo, RepoPermission, RepoSettings, -}; +use crate::github::api::{GithubRead, Login, PushAllowanceActor, RepoPermission, RepoSettings}; use log::debug; use rust_team_data::v1::Bot; use std::collections::{HashMap, HashSet}; @@ -28,7 +26,7 @@ pub(crate) fn create_diff( type OrgName = String; type RepoName = String; -#[derive(Clone, Debug, PartialEq)] +#[derive(Copy, Clone, Debug, PartialEq)] enum GithubApp { RenovateBot, } @@ -91,7 +89,7 @@ impl SyncGitHub { let usernames_cache = github.usernames(&users)?; debug!("caching organization owners"); - let mut orgs = teams + let orgs = teams .iter() .filter_map(|t| t.github.as_ref()) .flat_map(|gh| &gh.teams) @@ -334,13 +332,39 @@ impl SyncGitHub { archived: expected_repo.archived, auto_merge_enabled: expected_repo.auto_merge_enabled, }; + + let existing_installations = self + .org_apps + .get(&expected_repo.org) + .map(|installations| { + installations + .iter() + .filter_map(|installation| { + // Only load installations from apps that we know about, to avoid removing + // unknown installations. + if installation.repositories.contains(&actual_repo.name) { + Some(AppInstallation { + app: installation.app, + installation_id: installation.installation_id, + }) + } else { + None + } + }) + .collect::>() + }) + .unwrap_or_default(); + let app_installation_diffs = + self.diff_app_installations(expected_repo, &existing_installations)?; Ok(RepoDiff::Update(UpdateRepoDiff { org: expected_repo.org.clone(), name: actual_repo.name, repo_node_id: actual_repo.node_id, + repo_id: actual_repo.repo_id, settings_diff: (old_settings, new_settings), permission_diffs, branch_protection_diffs, + app_installation_diffs, })) } @@ -703,7 +727,7 @@ impl CreateRepoDiff { } for installation in &self.app_installations { - installation.apply(sync, &repo)?; + installation.apply(sync, repo.repo_id)?; } Ok(()) @@ -746,10 +770,12 @@ struct UpdateRepoDiff { org: String, name: String, repo_node_id: String, + repo_id: u64, // old, new settings_diff: (RepoSettings, RepoSettings), permission_diffs: Vec, branch_protection_diffs: Vec, + app_installation_diffs: Vec, } impl UpdateRepoDiff { @@ -761,6 +787,7 @@ impl UpdateRepoDiff { self.settings_diff.0 == self.settings_diff.1 && self.permission_diffs.is_empty() && self.branch_protection_diffs.is_empty() + && self.app_installation_diffs.is_empty() } fn can_be_modified(&self) -> bool { @@ -788,6 +815,10 @@ impl UpdateRepoDiff { for branch_protection in &self.branch_protection_diffs { branch_protection.apply(sync, &self.org, &self.name, &self.repo_node_id)?; } + + for app_installation in &self.app_installation_diffs { + app_installation.apply(sync, self.repo_id)?; + } Ok(()) } } @@ -841,6 +872,10 @@ impl std::fmt::Display for UpdateRepoDiff { for branch_protection_diff in &self.branch_protection_diffs { write!(f, "{branch_protection_diff}")?; } + writeln!(f, " App installation changes:")?; + for diff in &self.app_installation_diffs { + write!(f, "{diff}")?; + } Ok(()) } @@ -1010,13 +1045,13 @@ enum AppInstallationDiff { } impl AppInstallationDiff { - fn apply(&self, sync: &GitHubWrite, repo: &Repo) -> anyhow::Result<()> { + fn apply(&self, sync: &GitHubWrite, repo_id: u64) -> anyhow::Result<()> { match self { AppInstallationDiff::Add(app) => { - sync.add_repo_to_app_installation(app.installation_id, repo.repo_id)?; + sync.add_repo_to_app_installation(app.installation_id, repo_id)?; } - AppInstallationDiff::Remove(_) => { - todo!() + AppInstallationDiff::Remove(app) => { + sync.remove_repo_from_app_installation(app.installation_id, repo_id)?; } } Ok(()) From 04ba69b212142db84ff03097513a3fb5e25ba542 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 17 Mar 2024 18:12:41 +0100 Subject: [PATCH 9/9] Fix tests --- src/github/tests/test_utils.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/github/tests/test_utils.rs b/src/github/tests/test_utils.rs index bbb0990..b7b8f54 100644 --- a/src/github/tests/test_utils.rs +++ b/src/github/tests/test_utils.rs @@ -4,7 +4,8 @@ use derive_builder::Builder; use rust_team_data::v1::{GitHubTeam, Person, TeamGitHub, TeamKind}; use crate::github::api::{ - BranchProtection, GithubRead, Repo, RepoTeam, RepoUser, Team, TeamMember, TeamPrivacy, TeamRole, + BranchProtection, GithubRead, OrgAppInstallation, Repo, RepoAppInstallation, RepoTeam, + RepoUser, Team, TeamMember, TeamPrivacy, TeamRole, }; use crate::github::{api, SyncGitHub, TeamDiff}; @@ -217,6 +218,17 @@ impl GithubRead for GithubMock { .collect()) } + fn org_app_installations(&self, _org: &str) -> anyhow::Result> { + Ok(vec![]) + } + + fn app_installation_repos( + &self, + _installation_id: u64, + ) -> anyhow::Result> { + Ok(vec![]) + } + fn org_teams(&self, org: &str) -> anyhow::Result> { assert_eq!(org, DEFAULT_ORG); Ok(self