Merge pull request #113 from vohoanglong0107/refactor-get-repository-name

refactor: unify ways to get repository name

Author: Kobzol

src/bors/handlers/mod.rs

@@ -139,7 +139,7 @@ pub async fn handle_bors_global_event(
             futures::future::join_all(repos.into_iter().map(|repo| {
                 let repo = Arc::clone(&repo);
                 async {
-                    let subspan = tracing::info_span!("Repo", repo = repo.repository.to_string());
+                    let subspan = tracing::info_span!("Repo", repo = repo.repository().to_string());
                     refresh_repository(repo, Arc::clone(&db), team_api_client)
                         .instrument(subspan)
                         .await
@@ -249,8 +249,8 @@ async fn reload_repos(
     let reloaded_repos = repo_loader.load_repositories(team_api_client).await?;
     let mut repositories = ctx.repositories.write().unwrap();
     for repo in repositories.values() {
-        if !reloaded_repos.contains_key(&repo.repository) {
-            tracing::warn!("Repository {} was not reloaded", repo.repository);
+        if !reloaded_repos.contains_key(repo.repository()) {
+            tracing::warn!("Repository {} was not reloaded", repo.repository());
         }
     }
     for (name, repo) in reloaded_repos {

src/bors/handlers/refresh.rs

@@ -29,7 +29,7 @@ pub async fn refresh_repository(
 }
 
 async fn cancel_timed_out_builds(repo: &RepositoryState, db: &PgDbClient) -> anyhow::Result<()> {
-    let running_builds = db.get_running_builds(&repo.repository).await?;
+    let running_builds = db.get_running_builds(repo.repository()).await?;
     tracing::info!("Found {} running build(s)", running_builds.len());
 
     let timeout = repo.config.load().timeout;
@@ -67,12 +67,12 @@ async fn reload_permission(
     team_api_client: &TeamApiClient,
 ) -> anyhow::Result<()> {
     let permissions = team_api_client
-        .load_permissions(&repo.repository)
+        .load_permissions(repo.repository())
         .await
         .with_context(|| {
             format!(
                 "Could not load permissions for repository {}",
-                repo.repository
+                repo.repository()
             )
         })?;
     repo.permissions.store(Arc::new(permissions));

src/bors/mod.rs

@@ -60,8 +60,13 @@ pub struct CheckSuite {
 /// Can be used to query permissions for the repository, and also to perform various
 /// actions using the stored client.
 pub struct RepositoryState {
-    pub repository: GithubRepoName,
     pub client: GithubRepositoryClient,
     pub permissions: ArcSwap<UserPermissions>,
     pub config: ArcSwap<RepositoryConfig>,
 }
+
+impl RepositoryState {
+    pub fn repository(&self) -> &GithubRepoName {
+        self.client.repository()
+    }
+}

src/github/api/client.rs

@@ -13,27 +13,37 @@ use crate::github::{Branch, CommitSha, GithubRepoName, PullRequest, PullRequestN
 
 /// Provides access to a single app installation (repository) using the GitHub API.
 pub struct GithubRepositoryClient {
-    pub app: App,
+    app: App,
     /// The client caches the access token for this given repository and refreshes it once it
     /// expires.
-    pub client: Octocrab,
+    client: Octocrab,
     // We store the name separately, because repository has an optional owner, but at this point
     // we must always have some owner of the repo.
-    pub repo_name: GithubRepoName,
-    pub repository: Repository,
+    repo_name: GithubRepoName,
+    repository: Repository,
 }
 
 impl GithubRepositoryClient {
-    pub fn client(&self) -> &Octocrab {
-        &self.client
+    pub fn new(
+        app: App,
+        client: Octocrab,
+        repo_name: GithubRepoName,
+        repository: Repository,
+    ) -> Self {
+        Self {
+            app,
+            client,
+            repo_name,
+            repository,
+        }
     }
 
-    pub fn name(&self) -> &GithubRepoName {
-        &self.repo_name
+    pub fn client(&self) -> &Octocrab {
+        &self.client
     }
 
     pub fn repository(&self) -> &GithubRepoName {
-        self.name()
+        &self.repo_name
     }
 
     /// Was the comment created by the bot?
@@ -78,12 +88,7 @@ impl GithubRepositoryClient {
         let branch: octocrab::models::repos::Branch = self
             .client
             .get(
-                format!(
-                    "/repos/{}/{}/branches/{name}",
-                    self.repo_name.owner(),
-                    self.repo_name.name(),
-                )
-                .as_str(),
+                format!("/repos/{}/branches/{name}", self.repository()).as_str(),
                 None::<&()>,
             )
             .await
@@ -112,7 +117,7 @@ impl GithubRepositoryClient {
         comment: Comment,
     ) -> anyhow::Result<()> {
         self.client
-            .issues(&self.name().owner, &self.name().name)
+            .issues(&self.repository().owner, &self.repository().name)
             .create_comment(pr.0, comment.render())
             .await
             .with_context(|| format!("Cannot post comment to {}", self.format_pr(pr)))?;
@@ -215,7 +220,9 @@ impl GithubRepositoryClient {
 
     /// Add a set of labels to a PR.
     pub async fn add_labels(&self, pr: PullRequestNumber, labels: &[String]) -> anyhow::Result<()> {
-        let client = self.client.issues(self.name().owner(), self.name().name());
+        let client = self
+            .client
+            .issues(self.repository().owner(), self.repository().name());
         if !labels.is_empty() {
             client
                 .add_labels(pr.0, labels)
@@ -232,7 +239,9 @@ impl GithubRepositoryClient {
         pr: PullRequestNumber,
         labels: &[String],
     ) -> anyhow::Result<()> {
-        let client = self.client.issues(self.name().owner(), self.name().name());
+        let client = self
+            .client
+            .issues(self.repository().owner(), self.repository().name());
         // The GitHub API only allows removing labels one by one, so we remove all of them in
         // parallel to speed it up a little.
         let labels_to_remove_futures = labels.iter().map(|label| client.remove_label(pr.0, label));
@@ -266,19 +275,12 @@ impl GithubRepositoryClient {
             .html_url
             .as_ref()
             .map(|url| url.to_string())
-            .unwrap_or_else(|| {
-                format!(
-                    "{}/{}/{}",
-                    base_github_html_url(),
-                    self.name().owner,
-                    self.name().name
-                )
-            });
+            .unwrap_or_else(|| format!("{}/{}", base_github_html_url(), self.repository(),));
         format!("{html_url}/actions/runs/{run_id}")
     }
 
     fn format_pr(&self, pr: PullRequestNumber) -> String {
-        format!("{}/{}/{}", self.name().owner(), self.name().name(), pr)
+        format!("{}/{}", self.repository(), pr)
     }
 }
 

src/github/api/mod.rs

@@ -143,12 +143,7 @@ async fn create_repo_state(
 ) -> anyhow::Result<RepositoryState> {
     tracing::info!("Found repository {name}");
 
-    let client = GithubRepositoryClient {
-        app,
-        client: repo_client,
-        repo_name: name.clone(),
-        repository: repo,
-    };
+    let client = GithubRepositoryClient::new(app, repo_client, name.clone(), repo);
 
     let permissions = team_api_client
         .load_permissions(&name)
@@ -168,7 +163,6 @@ async fn create_repo_state(
     };
 
     Ok(RepositoryState {
-        repository: name,
         client,
         config: ArcSwap::new(Arc::new(config)),
         permissions: ArcSwap::new(Arc::new(permissions)),

src/github/api/operations.rs

@@ -41,7 +41,7 @@ pub async fn merge_branches(
     commit_message: &str,
 ) -> Result<CommitSha, MergeError> {
     let client = repo.client();
-    let merge_url = format!("/repos/{}/{}/merges", repo.name().owner, repo.name().name);
+    let merge_url = format!("/repos/{}/merges", repo.repository());
 
     let request = MergeRequest {
         base: base_ref,
@@ -57,7 +57,7 @@ pub async fn merge_branches(
 
             tracing::trace!(
                 "Response from merging `{head_sha}` into `{base_ref}` in `{}`: {status} ({text})",
-                repo.name(),
+                repo.repository(),
             );
 
             match status {
@@ -79,7 +79,7 @@ pub async fn merge_branches(
         Err(error) => {
             tracing::debug!(
                 "Merging `{head_sha}` into `{base_ref}` in `{}` failed: {error:?}",
-                repo.name()
+                repo.repository()
             );
             Err(MergeError::NetworkError(error))
         }
@@ -112,8 +112,8 @@ async fn create_branch(
     name: String,
     sha: &CommitSha,
 ) -> Result<(), String> {
-    repo.client
-        .repos(repo.repo_name.owner(), repo.repo_name.name())
+    repo.client()
+        .repos(repo.repository().owner(), repo.repository().name())
         .create_ref(&Reference::Branch(name), sha.as_ref())
         .await
         .map_err(|error| format!("Cannot create branch: {error}"))?;
@@ -137,16 +137,15 @@ async fn update_branch(
     sha: &CommitSha,
 ) -> Result<(), BranchUpdateError> {
     let url = format!(
-        "/repos/{}/{}/git/refs/{}",
-        repo.name().owner(),
-        repo.name().name(),
+        "/repos/{}/git/refs/{}",
+        repo.repository(),
         Reference::Branch(branch_name.clone()).ref_url()
     );
 
     tracing::debug!("Updating branch {} to SHA {}", url, sha.as_ref());
 
     let res = repo
-        .client
+        .client()
         ._patch(
             url.as_str(),
             Some(&serde_json::json!({
@@ -160,7 +159,7 @@ async fn update_branch(
     tracing::trace!(
         "Updating branch response: status={}, text={:?}",
         status,
-        repo.client.body_to_string(res).await
+        repo.client().body_to_string(res).await
     );
 
     match status {