From 7c6abecd110eb60b2eecf535944943f7199d4cac Mon Sep 17 00:00:00 2001 From: Meysam Azad Date: Mon, 23 Oct 2023 13:43:57 +0700 Subject: [PATCH] wip: cache the diffs if not commit was pushed --- src/config.rs | 8 +++-- src/github.rs | 3 +- src/handlers/validate_config.rs | 25 ++++++++++++-- src/handlers/validate_config/lifo.rs | 50 ++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 src/handlers/validate_config/lifo.rs diff --git a/src/config.rs b/src/config.rs index 4e1882c0..0b3d6233 100644 --- a/src/config.rs +++ b/src/config.rs @@ -160,11 +160,15 @@ pub(crate) struct PrioritizeConfig { } #[derive(PartialEq, Eq, Debug, serde::Deserialize)] -pub(crate) struct ValidateConfig {} +pub(crate) struct ValidateConfig { + diff_history_size: usize, +} impl ValidateConfig { fn default() -> Option { - Some(ValidateConfig {}) + Some(ValidateConfig { + diff_history_size: 128, + }) } } diff --git a/src/github.rs b/src/github.rs index 59fb6326..c36b8b10 100644 --- a/src/github.rs +++ b/src/github.rs @@ -846,11 +846,12 @@ impl Issue { } } -#[derive(Debug, serde::Deserialize)] +#[derive(Debug, serde::Deserialize, Clone)] pub struct PullRequestFile { pub sha: String, pub filename: String, pub blob_url: String, + pub raw_url: String, } #[derive(serde::Serialize)] diff --git a/src/handlers/validate_config.rs b/src/handlers/validate_config.rs index 3283aad2..1909eb5d 100644 --- a/src/handlers/validate_config.rs +++ b/src/handlers/validate_config.rs @@ -1,8 +1,26 @@ //! For pull requests that have changed the triagebot.toml, validate that the //! changes are a valid configuration file. //! It won't validate anything unless the PR is open and has changed. -//! For every event, it will fetch the diff of the pull request which is one -//! round-trip of HTTP request to the GitHub API. +//! +//! 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. +//! use crate::{ config::ValidateConfig, @@ -11,6 +29,9 @@ use crate::{ }; use tracing as log; +mod lifo; +use lifo::LifoHashMap; + /// If the issue is a pull request and if among the changed files, triagebot.toml /// is changed, then validate the triagebot.toml config file with the syntax /// parser from the config module. diff --git a/src/handlers/validate_config/lifo.rs b/src/handlers/validate_config/lifo.rs new file mode 100644 index 00000000..37b65da6 --- /dev/null +++ b/src/handlers/validate_config/lifo.rs @@ -0,0 +1,50 @@ +use std::collections::HashMap; + +pub(super) struct LifoHashMap { + capacity: usize, + entries: Vec>, + map: HashMap, +} + +impl LifoHashMap +where + K: Eq + std::hash::Hash + std::clone::Clone, + V: std::clone::Clone, +{ + pub fn new(capacity: usize) -> Self { + Self { + capacity, + entries: vec![None; capacity], + map: HashMap::with_capacity(capacity), + } + } + + pub fn push(&mut self, key: K, value: V) { + if self.map.len() >= self.capacity { + if let Some(oldest) = self.entries.pop() { + if let Some((oldest_key, _)) = oldest { + self.map.remove(&oldest_key); + } + } + } + + self.entries.insert(0, Some((key.clone(), value))); + self.map.insert(key, 0); + } + + pub fn get(&self, key: &K) -> Option<&V> { + if let Some(index) = self.map.get(key) { + if let Some(entry) = &self.entries[*index] { + return Some(&entry.1); + } + } + None + } + + pub fn remove(&mut self, key: &K) { + if let Some(index) = self.map.get(key) { + self.entries[*index] = None; + self.map.remove(key); + } + } +}