Merge pull request #14 from TicClick/quickfix-fetching

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "observatory"
-version = "0.2.7"
+version = "0.2.8"
 edition = "2021"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

src/controller/controller_impl.rs

@@ -150,31 +150,32 @@ impl<T: GitHubInterface> Controller<T> {
     /// Build the in-memory pull request cache on start-up. This will consume a lot of GitHub API quota,
     /// but fighting a stale database cache is left as an exercise for another day.
     async fn init(&mut self) -> Result<()> {
-        self.app = Some(self.github.app().await?);
+        self.app = Some(self.github.read_app().await?);
         log::info!("GitHub application: {:?}", self.app.as_ref().unwrap());
 
-        let installations = self.github.discover_installations().await?;
+        let installations = self.github.read_installations().await?;
         log::info!("Active installations: {:?}", installations);
         for i in installations {
-            self.add_repositories(i.id, self.github.cached_repositories(i.id))
-                .await;
+            let id = i.id;
+            match self.add_installation(i).await {
+                Err(e) => log::error!("Failed to add installation {}: {}", id, e),
+                Ok(_) => log::info!("Processed repositories from installation {}", id)
+            }
         }
         Ok(())
     }
 
     /// Add an installation and fetch pull requests (one installation may have several repos).
     async fn add_installation(&self, installation: Installation) -> Result<()> {
         let iid = installation.id;
-        self.github.add_installation(installation).await?;
+        self.github.read_and_cache_installation_repos(installation).await?;
         self.add_repositories(iid, self.github.cached_repositories(iid))
             .await;
         Ok(())
     }
 
     /// Add several repositories the app just got an access to.
     async fn add_repositories(&self, installation_id: i64, repositories: Vec<Repository>) {
-        self.github
-            .add_repositories(installation_id, repositories.clone());
         for r in repositories {
             log::debug!(
                 "Adding repository {:?} for installation #{}",
@@ -194,7 +195,7 @@ impl<T: GitHubInterface> Controller<T> {
 
     /// Add a repository and fetch its pull requests.
     async fn add_repository(&self, r: &Repository) -> Result<()> {
-        for p in self.github.pulls(&r.full_name).await? {
+        for p in self.github.read_pulls(&r.full_name).await? {
             self.upsert_pull(&r.full_name, p, false).await?;
         }
         Ok(())
@@ -339,7 +340,7 @@ impl<T: GitHubInterface> Controller<T> {
         for pull_number in pending.keys().chain(to_remove.keys()) {
             let existing_comments = self
                 .github
-                .list_comments(full_repo_name, *pull_number)
+                .read_comments(full_repo_name, *pull_number)
                 .await?
                 .into_iter()
                 .filter(|c| self.has_control_over(&c.user));

src/controller/controller_impl/tests/tests_comments.rs

@@ -15,7 +15,7 @@ async fn test_no_conflict_no_comment() {
     .unwrap();
     let comments = c
         .github
-        .list_comments("test/repo", p1.number)
+        .read_comments("test/repo", p1.number)
         .await
         .unwrap();
     assert!(comments.is_empty());
@@ -44,14 +44,14 @@ async fn test_one_conflict_one_comment() {
 
     let first_pull_comments = c
         .github
-        .list_comments("test/repo", p1.number)
+        .read_comments("test/repo", p1.number)
         .await
         .unwrap();
     assert!(first_pull_comments.is_empty());
 
     let second_pull_comments = c
         .github
-        .list_comments("test/repo", p2.number)
+        .read_comments("test/repo", p2.number)
         .await
         .unwrap();
     assert_eq!(second_pull_comments.len(), 1);
@@ -76,7 +76,7 @@ async fn test_one_conflict_one_valid_header() {
 
     let second_pull_comments = c
         .github
-        .list_comments("test/repo", pulls[1].number)
+        .read_comments("test/repo", pulls[1].number)
         .await
         .unwrap();
     let header = CommentHeader::from_comment(&second_pull_comments.first().unwrap().body).unwrap();
@@ -134,7 +134,7 @@ async fn test_one_pull_and_conflict_one_comment() {
 
     let second_pull_comments = c
         .github
-        .list_comments("test/repo", pulls[1].number)
+        .read_comments("test/repo", pulls[1].number)
         .await
         .unwrap();
     assert_eq!(second_pull_comments.len(), 1);
@@ -185,7 +185,7 @@ async fn test_one_pull_and_conflict_one_comment_updated() {
 
     let second_pull_comments = c
         .github
-        .list_comments("test/repo", pulls[1].number)
+        .read_comments("test/repo", pulls[1].number)
         .await
         .unwrap();
     let only_comment = &second_pull_comments.first().unwrap().body;
@@ -234,7 +234,7 @@ async fn test_post_comment_per_pull_and_conflict_combination() {
 
     let third_pull_comments = c
         .github
-        .list_comments("test/repo", pulls[2].number)
+        .read_comments("test/repo", pulls[2].number)
         .await
         .unwrap();
     assert_eq!(third_pull_comments.len(), 4);
@@ -291,7 +291,7 @@ async fn test_obsolete_comment_is_removed() {
         .unwrap();
     assert!(c
         .github
-        .list_comments("test/repo", 2)
+        .read_comments("test/repo", 2)
         .await
         .unwrap()
         .is_empty())
@@ -321,7 +321,7 @@ async fn test_only_target_comment_is_removed() {
     }
 
     assert_eq!(
-        c.github.list_comments("test/repo", 2).await.unwrap().len(),
+        c.github.read_comments("test/repo", 2).await.unwrap().len(),
         2
     );
 
@@ -331,7 +331,7 @@ async fn test_only_target_comment_is_removed() {
         .await
         .unwrap();
 
-    let comments = c.github.list_comments("test/repo", 2).await.unwrap();
+    let comments = c.github.read_comments("test/repo", 2).await.unwrap();
     assert_eq!(comments.len(), 1);
     let h = CommentHeader::from_comment(&comments.first().unwrap().body).unwrap();
     assert_eq!(
@@ -361,7 +361,7 @@ async fn test_new_comment_is_posted_after_removal_in_different_pull() {
     }
 
     assert_eq!(
-        c.github.list_comments("test/repo", 2).await.unwrap().len(),
+        c.github.read_comments("test/repo", 2).await.unwrap().len(),
         1
     );
 
@@ -375,7 +375,7 @@ async fn test_new_comment_is_posted_after_removal_in_different_pull() {
 
     assert!(c
         .github
-        .list_comments("test/repo", 2)
+        .read_comments("test/repo", 2)
         .await
         .unwrap()
         .is_empty());
@@ -386,7 +386,7 @@ async fn test_new_comment_is_posted_after_removal_in_different_pull() {
         .await
         .unwrap();
 
-    let comments = c.github.list_comments("test/repo", 1).await.unwrap();
+    let comments = c.github.read_comments("test/repo", 1).await.unwrap();
     assert_eq!(comments.first().unwrap().id, 2);
     assert!(comments.first().unwrap().created_at > first_conflict_created_at);
 }

src/controller/controller_impl/tests/tests_installations_repos.rs

@@ -18,7 +18,7 @@ async fn test_add_installations() {
         c.add_installation(inst.clone()).await.unwrap();
         installations.push(inst);
     }
-    let mut v = c.github.installations().await.unwrap();
+    let mut v = c.github.read_installations().await.unwrap();
     v.sort_by_key(|i| i.id);
     assert_eq!(v, installations);
 }

src/github.rs

@@ -144,19 +144,18 @@ impl Token {
 #[async_trait]
 pub trait GitHubInterface {
     fn new(app_id: String, key: String) -> Self;
-    async fn installations(&self) -> Result<Vec<structs::Installation>>;
+    async fn read_installations(&self) -> Result<Vec<structs::Installation>>;
     fn cached_installations(&self) -> Vec<structs::Installation>;
-    fn add_repositories(&self, installation_id: i64, repositories: Vec<structs::Repository>);
+    fn cache_repositories(&self, installation_id: i64, repositories: Vec<structs::Repository>);
     fn remove_repositories(&self, installation_id: i64, repositories: &[structs::Repository]);
-    async fn discover_installations(&self) -> Result<Vec<structs::Installation>>;
-    async fn app(&self) -> Result<structs::App>;
-    async fn add_installation(
+    async fn read_app(&self) -> Result<structs::App>;
+    async fn read_and_cache_installation_repos(
         &self,
         mut installation: structs::Installation,
     ) -> Result<structs::Installation>;
     fn remove_installation(&self, installation: &structs::Installation);
     fn cached_repositories(&self, installation_id: i64) -> Vec<structs::Repository>;
-    async fn pulls(&self, full_repo_name: &str) -> Result<Vec<structs::PullRequest>>;
+    async fn read_pulls(&self, full_repo_name: &str) -> Result<Vec<structs::PullRequest>>;
     async fn post_comment(
         &self,
         full_repo_name: &str,
@@ -170,7 +169,7 @@ pub trait GitHubInterface {
         body: String,
     ) -> Result<()>;
     async fn delete_comment(&self, full_repo_name: &str, comment_id: i64) -> Result<()>;
-    async fn list_comments(
+    async fn read_comments(
         &self,
         full_repo_name: &str,
         issue_number: i32,
@@ -410,7 +409,7 @@ impl GitHubInterface for Client {
         }
     }
 
-    async fn app(&self) -> Result<structs::App> {
+    async fn read_app(&self) -> Result<structs::App> {
         let pp = self
             .http_client
             .get(GitHub::app())
@@ -428,7 +427,7 @@ impl GitHubInterface for Client {
             .collect()
     }
 
-    fn add_repositories(&self, installation_id: i64, mut repositories: Vec<structs::Repository>) {
+    fn cache_repositories(&self, installation_id: i64, mut repositories: Vec<structs::Repository>) {
         if let Some(repos) = self.repos.lock().unwrap().get_mut(&installation_id) {
             let ids: Vec<_> = repositories.iter().map(|r| r.id).collect();
             repos.retain(|r| !ids.contains(&r.id));
@@ -443,8 +442,7 @@ impl GitHubInterface for Client {
         }
     }
 
-    // TODO: confirm that this is actually needed (see similar stuff below)
-    async fn installations(&self) -> Result<Vec<structs::Installation>> {
+    async fn read_installations(&self) -> Result<Vec<structs::Installation>> {
         let pp = self
             .http_client
             .get(GitHub::app_installations())
@@ -453,17 +451,7 @@ impl GitHubInterface for Client {
         Ok(items)
     }
 
-    async fn discover_installations(&self) -> Result<Vec<structs::Installation>> {
-        let mut ret = Vec::new();
-        if let Ok(installations) = self.installations().await {
-            for installation in installations {
-                ret.push(self.add_installation(installation).await?);
-            }
-        }
-        Ok(ret)
-    }
-
-    async fn add_installation(
+    async fn read_and_cache_installation_repos(
         &self,
         installation: structs::Installation,
     ) -> Result<structs::Installation> {
@@ -477,6 +465,7 @@ impl GitHubInterface for Client {
                 Err(e)
             }
             Ok(token) => {
+                self.repos.lock().unwrap().insert(installation.id, Vec::new());
                 let req = self
                     .http_client
                     .get(GitHub::installation_repos())
@@ -487,7 +476,7 @@ impl GitHubInterface for Client {
                         Err(e)
                     }
                     Ok(response) => {
-                        self.add_repositories(installation.id, response.repositories);
+                        self.cache_repositories(installation.id, response.repositories);
                         Ok(installation)
                     }
                 }
@@ -511,7 +500,7 @@ impl GitHubInterface for Client {
             .remove(&TokenType::Installation(installation.id));
     }
 
-    async fn pulls(&self, full_repo_name: &str) -> Result<Vec<structs::PullRequest>> {
+    async fn read_pulls(&self, full_repo_name: &str) -> Result<Vec<structs::PullRequest>> {
         let mut out = Vec::new();
         let token = self.pick_token(full_repo_name).await?;
         let per_page = 100;
@@ -582,7 +571,7 @@ impl GitHubInterface for Client {
         Ok(())
     }
 
-    async fn list_comments(
+    async fn read_comments(
         &self,
         full_repo_name: &str,
         issue_number: i32,

src/test.rs

@@ -91,7 +91,7 @@ impl github::GitHubInterface for DummyGitHubClient {
         }
     }
 
-    async fn installations(&self) -> Result<Vec<structs::Installation>> {
+    async fn read_installations(&self) -> Result<Vec<structs::Installation>> {
         Ok(self.cached_installations())
     }
 
@@ -104,7 +104,7 @@ impl github::GitHubInterface for DummyGitHubClient {
             .collect()
     }
 
-    fn add_repositories(&self, installation_id: i64, mut repositories: Vec<structs::Repository>) {
+    fn cache_repositories(&self, installation_id: i64, mut repositories: Vec<structs::Repository>) {
         if let Some(repos) = self.repos.lock().unwrap().get_mut(&installation_id) {
             let ids: Vec<_> = repos.iter().map(|r| r.id).collect();
             repos.retain(|r| !ids.contains(&r.id));
@@ -119,11 +119,7 @@ impl github::GitHubInterface for DummyGitHubClient {
         }
     }
 
-    async fn discover_installations(&self) -> Result<Vec<structs::Installation>> {
-        Ok(self.cached_installations())
-    }
-
-    async fn app(&self) -> Result<structs::App> {
+    async fn read_app(&self) -> Result<structs::App> {
         Ok(structs::App {
             id: self.app_id.parse().unwrap(),
             slug: "test-app".to_string(),
@@ -135,7 +131,7 @@ impl github::GitHubInterface for DummyGitHubClient {
         })
     }
 
-    async fn add_installation(
+    async fn read_and_cache_installation_repos(
         &self,
         installation: structs::Installation,
     ) -> Result<structs::Installation> {
@@ -158,7 +154,7 @@ impl github::GitHubInterface for DummyGitHubClient {
         self.repos.lock().unwrap().remove(&installation.id);
     }
 
-    async fn pulls(&self, full_repo_name: &str) -> Result<Vec<structs::PullRequest>> {
+    async fn read_pulls(&self, full_repo_name: &str) -> Result<Vec<structs::PullRequest>> {
         match self.pulls.lock().unwrap().get(&full_repo_name.to_string()) {
             Some(v) => Ok(v.clone()),
             None => Ok(Vec::new()),
@@ -233,7 +229,7 @@ impl github::GitHubInterface for DummyGitHubClient {
         Ok(())
     }
 
-    async fn list_comments(
+    async fn read_comments(
         &self,
         full_repo_name: &str,
         issue_number: i32,