diff --git a/src/config.rs b/src/config.rs index 2c81b9888..5221341bd 100644 --- a/src/config.rs +++ b/src/config.rs @@ -161,15 +161,11 @@ pub(crate) struct PrioritizeConfig { } #[derive(PartialEq, Eq, Debug, serde::Deserialize)] -pub(crate) struct ValidateConfig { - diff_history_size: usize, -} +pub(crate) struct ValidateConfig {} impl ValidateConfig { fn default() -> Option { - Some(ValidateConfig { - diff_history_size: 128, - }) + Some(ValidateConfig {}) } } @@ -433,7 +429,7 @@ mod tests { note: Some(NoteConfig { _empty: () }), ping: Some(PingConfig { teams: ping_teams }), nominate: Some(NominateConfig { - teams: nominate_teams + teams: nominate_teams, }), shortcut: Some(ShortcutConfig { _empty: () }), prioritize: None, diff --git a/src/github.rs b/src/github.rs index 373437efa..749d93591 100644 --- a/src/github.rs +++ b/src/github.rs @@ -977,7 +977,7 @@ struct PullRequestEventFields {} #[derive(Clone, Debug, serde::Deserialize)] pub struct CommitBase { - sha: String, + pub sha: String, #[serde(rename = "ref")] pub git_ref: String, pub repo: Repository, diff --git a/src/handlers/validate_config.rs b/src/handlers/validate_config.rs index 1909eb5d1..f1edaf085 100644 --- a/src/handlers/validate_config.rs +++ b/src/handlers/validate_config.rs @@ -3,32 +3,24 @@ //! It won't validate anything unless the PR is open and has changed. //! //! These are the implementation steps as follows: -//! 1. Fetch the diff using the following url: -//! https://api.github.com/repos/rust-lang/triagebot/compare/master...meysam81:master -//! 2. The last step will return a JSON response with the `files` field as an -//! array of objects, each having `filename` in their fields. We check if -//! `triagebot.toml` is among them, and continue to the next step. Otherwise -//! we break out of the validation process. -//! 3. Fetch the raw file from the head branch with the secon HTTP request -//! (two network calls so far): -//! https://raw.githubusercontent.com/meysam81/triagebot/master/triagebot.toml -//! 4. Use the `toml` crate and the `Config` module to parse **and** validate -//! the contents of the file. -//! 5. This last step will return error if the file is not a valid config file. -//! -//! Considerations: -//! - HTTP calls are network requests, prone to failure and expensive in nature -//! which is why we are using a fixed LIFO hash map to store the value if the -//! URL hasn't changed e.g. no new commits has been pushed. -//! +//! 1. If the issue is of type pull request and it is open, then continue. +//! 2. Fetch the triagebot.toml from the HEAD branch of the PR. +//! 3. If the triagebot.toml is valid, then continue. +//! 4. If the triagebot.toml is invalid, then report the error. + +use crate::{ config::ValidateConfig, github::IssuesAction, handlers::{ Context, IssuesEvent } }; -use crate::{ - config::ValidateConfig, - github::IssuesAction, - handlers::{Context, IssuesEvent}, -}; +use once_cell::sync::Lazy; +use std::{ sync::Mutex }; use tracing as log; +/// Caching the content of triagebot.toml across commit SHA to save network round-trip +/// time. The content will look like this: +/// COMMIT_SHA => CONTENT_OF_TRIAGEBOT_TOML +static TRIAGEBOT_CACHE: Lazy>>> = Lazy::new(|| + Mutex::new(LifoHashMap::new(128)) +); + mod lifo; use lifo::LifoHashMap; @@ -38,89 +30,89 @@ use lifo::LifoHashMap; pub(super) async fn parse_input( ctx: &Context, event: &IssuesEvent, - _config: Option<&ValidateConfig>, + _config: Option<&ValidateConfig> ) -> Result, String> { if !event.issue.is_pr() { log::debug!("Ignoring issue: {:?}", event.issue); return Ok(None); } - if !matches!( - event.action, - IssuesAction::Opened | IssuesAction::Synchronize | IssuesAction::ReadyForReview, - ) { + if + !matches!( + event.action, + IssuesAction::Opened | IssuesAction::Synchronize | IssuesAction::ReadyForReview + ) + { log::debug!("Ignoring issue action: {:?}", event.action); return Ok(None); } - let diff = match event.issue.diff(&ctx.github).await { - Ok(Some(diff)) => diff, - wildcard => { - log::error!("Failed to fetch diff: {:?}", wildcard); - return Ok(None); + log::debug!("Validating triagebot config"); + match validate_triagebot_config(ctx, event).await { + Ok(()) => { + log::debug!("Valid triagebot config"); + Ok(Some(())) } - }; - - if diff.contains(crate::config::CONFIG_FILE_NAME) { - log::debug!("Validating triagebot config"); - match validate_triagebot_config(ctx, event).await { - Ok(()) => { - log::debug!("Valid triagebot config"); - return Ok(Some(())); - } - Err(e) => { - log::error!("Invalid triagebot config: {:?}", e); - return Err(e); - } + Err(e) => { + log::error!("Invalid triagebot config: {:?}", e); + Err(e) } } - - Ok(None) } -async fn get_triagebot_config_blob_url(repo: &str, git_ref: &str) -> String { - // https://github.com/rust-lang/triagebot/blob/master/triagebot.toml - format!( - "https://github.com/{}/blob/{}/{}", +async fn get_triagebot_content(repo: &str, git_ref: &str, sha: &str) -> Vec { + // Try to hit the cache first + if let Some(triagebot_toml) = TRIAGEBOT_CACHE.lock().unwrap().get(&sha.to_string()) { + log::info!("Cache hit for triagebot.toml for repository {} and commit {}", repo, sha); + return triagebot_toml.clone(); + } + + log::warn!("Cache miss for triagebot.toml for repository {} and commit {}", repo, sha); + + let url = format!( + "https://raw.githubusercontent.com/{}/{}/{}", repo, git_ref, crate::config::CONFIG_FILE_NAME - ) + ); + + let triagebot_toml = reqwest::get(&url).await.unwrap().text().await.unwrap().into_bytes(); + + TRIAGEBOT_CACHE.lock().unwrap().push(sha.to_string(), triagebot_toml.clone()); + + triagebot_toml } -async fn validate_triagebot_config(ctx: &Context, event: &IssuesEvent) -> Result<(), String> { +async fn validate_triagebot_config(_ctx: &Context, event: &IssuesEvent) -> Result<(), String> { if let Some(pr_source) = &event.issue.head { - if let Ok(Some(triagebot_toml)) = ctx - .github - .raw_file( - &pr_source.repo.full_name, - &pr_source.git_ref, - crate::config::CONFIG_FILE_NAME, - ) - .await - { - match toml::from_slice::(&triagebot_toml) { - Ok(_) => return Ok(()), - Err(e) => { - let position = e - .line_col() - .map(|(line, col)| format!("{}:{}", line + 1, col + 1)); - - let absolute_url = format!( - "{}#L{}", - get_triagebot_config_blob_url( - &pr_source.repo.full_name, - &pr_source.git_ref - ) - .await, - position.clone().unwrap_or_default() - ); - - return Err(format!( + let triagebot_content = get_triagebot_content( + &pr_source.repo.full_name, + &pr_source.git_ref, + &pr_source.sha + ).await; + + match toml::from_slice::(&triagebot_content) { + Ok(_) => { + return Ok(()); + } + Err(e) => { + let position = e.line_col().map(|(line, col)| format!("{}:{}", line + 1, col + 1)); + + // https://github.com/rust-lang/triagebot/blob/master/triagebot.toml + let absolute_url = format!( + "https://github.com/{}/blob/{}/{}#L{}", + pr_source.repo.full_name, + pr_source.git_ref, + crate::config::CONFIG_FILE_NAME, + position.clone().unwrap_or_default() + ); + + return Err( + format!( "Invalid `triagebot.toml` at position {}", format!("[{}]({})", position.unwrap_or_default(), absolute_url) - )); - } + ) + ); } } } @@ -132,7 +124,7 @@ pub(super) async fn handle_input( _ctx: &Context, _config: &ValidateConfig, _event: &IssuesEvent, - _input: (), + _input: () ) -> anyhow::Result<()> { Ok(()) } diff --git a/src/handlers/validate_config/lifo.rs b/src/handlers/validate_config/lifo.rs index 37b65da6f..4ee44d265 100644 --- a/src/handlers/validate_config/lifo.rs +++ b/src/handlers/validate_config/lifo.rs @@ -40,11 +40,4 @@ where } None } - - pub fn remove(&mut self, key: &K) { - if let Some(index) = self.map.get(key) { - self.entries[*index] = None; - self.map.remove(key); - } - } }