From 6e121c77cc0b1c39f3efbaf981d8a9bb692ae735 Mon Sep 17 00:00:00 2001 From: Julian Date: Tue, 30 Aug 2022 13:40:12 -0300 Subject: [PATCH 01/15] Bring already implemented code --- .idea/workspace.xml | 90 ++++++++++ Cargo.lock | 66 ++++++-- Cargo.toml | 1 + parser/src/command.rs | 3 + parser/src/command/decision.rs | 188 +++++++++++++++++++++ src/handlers.rs | 2 + src/handlers/decision.rs | 211 ++++++++++++++++++++++++ test/decision/01_merging_proposal__1.md | 8 + test/decision/01_merging_proposal__2.md | 8 + test/decision/01_merging_proposal__3.md | 8 + 10 files changed, 571 insertions(+), 14 deletions(-) create mode 100644 .idea/workspace.xml create mode 100644 parser/src/command/decision.rs create mode 100644 src/handlers/decision.rs create mode 100644 test/decision/01_merging_proposal__1.md create mode 100644 test/decision/01_merging_proposal__2.md create mode 100644 test/decision/01_merging_proposal__3.md diff --git a/.idea/workspace.xml b/.idea/workspace.xml new file mode 100644 index 00000000..d52ebb7f --- /dev/null +++ b/.idea/workspace.xml @@ -0,0 +1,90 @@ + + + + + { + "useNewFormat": true +} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 1661637398588 + + + + + + + + + \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index 1036afd4..ffdfcb71 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -310,6 +310,16 @@ dependencies = [ "typenum", ] +[[package]] +name = "ctor" +version = "0.1.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cdffe87e1d521a10f9696f833fe502293ea446d7f256c06128293a4119bdf4cb" +dependencies = [ + "quote", + "syn", +] + [[package]] name = "cynic" version = "0.14.1" @@ -384,6 +394,12 @@ dependencies = [ "syn", ] +[[package]] +name = "diff" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8" + [[package]] name = "digest" version = "0.8.1" @@ -1182,6 +1198,15 @@ dependencies = [ "vcpkg", ] +[[package]] +name = "output_vt100" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "628223faebab4e3e40667ee0b2336d34a5b960ff60ea743ddfdbcf7770bcfb66" +dependencies = [ + "winapi", +] + [[package]] name = "parking_lot" version = "0.11.2" @@ -1385,13 +1410,25 @@ version = "0.2.16" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "eb9f9e6e233e5c4a35559a617bf40a4ec447db2e84c20b55a6f83167b7e57872" +[[package]] +name = "pretty_assertions" +version = "1.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c89f989ac94207d048d92db058e4f6ec7342b0971fc58d1271ca148b799b3563" +dependencies = [ + "ansi_term", + "ctor", + "diff", + "output_vt100", +] + [[package]] name = "proc-macro2" -version = "1.0.37" +version = "1.0.43" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ec757218438d5fda206afc041538b2f6d889286160d649a86a24d37e1235afd1" +checksum = "0a2ca2c61bc9f3d74d2886294ab7b9853abd9c1ad903a3ac7815c58989bb7bab" dependencies = [ - "unicode-xid", + "unicode-ident", ] [[package]] @@ -1408,9 +1445,9 @@ dependencies = [ [[package]] name = "quote" -version = "1.0.18" +version = "1.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a1feb54ed693b93a84e14094943b84b7c4eae204c512b7ccb95ab0c66d278ad1" +checksum = "bbe448f377a7d6961e30f5955f9b8d106c3f5e449d493ee1b125c1d43c2b5179" dependencies = [ "proc-macro2", ] @@ -1781,13 +1818,13 @@ checksum = "6bdef32e8150c2a081110b42772ffe7d7c9032b606bc226c8260fd97e0976601" [[package]] name = "syn" -version = "1.0.91" +version = "1.0.99" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b683b2b825c8eef438b77c36a06dc262294da3d5a5813fac20da149241dcd44d" +checksum = "58dbef6ec655055e20b86b15a8cc6d439cca19b667537ac6a1369572d151ab13" dependencies = [ "proc-macro2", "quote", - "unicode-xid", + "unicode-ident", ] [[package]] @@ -2100,6 +2137,7 @@ dependencies = [ "parser", "postgres-native-tls", "postgres-types", + "pretty_assertions", "rand", "regex", "reqwest", @@ -2224,6 +2262,12 @@ version = "0.3.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1a01404663e3db436ed2746d9fefef640d868edae3cceb81c3b8d5732fda678f" +[[package]] +name = "unicode-ident" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c4f5b37a154999a8f3f98cc23a628d850e154479cd94decf3414696e12e31aaf" + [[package]] name = "unicode-normalization" version = "0.1.19" @@ -2239,12 +2283,6 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3ed742d4ea2bd1176e236172c8429aaf54486e7ac098db29ffe6529e0ce50973" -[[package]] -name = "unicode-xid" -version = "0.2.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8ccb82d61f80a663efe1f787a51b16b5a51e3314d6ac365b08639f52387b33f3" - [[package]] name = "unicode_categories" version = "0.1.1" diff --git a/Cargo.toml b/Cargo.toml index 204fb06a..7cf33897 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,6 +44,7 @@ rand = "0.8.5" ignore = "0.4.18" postgres-types = { version = "0.2.4", features = ["derive"] } cron = { version = "0.12.0" } +pretty_assertions = "1.2" [dependencies.serde] version = "1" diff --git a/parser/src/command.rs b/parser/src/command.rs index 7e40949d..fbaa5dd2 100644 --- a/parser/src/command.rs +++ b/parser/src/command.rs @@ -13,6 +13,7 @@ pub mod prioritize; pub mod relabel; pub mod second; pub mod shortcut; +pub mod decision; #[derive(Debug, PartialEq)] pub enum Command<'a> { @@ -26,6 +27,7 @@ pub enum Command<'a> { Shortcut(Result>), Close(Result>), Note(Result>), + Decision(Result>), } #[derive(Debug)] @@ -207,6 +209,7 @@ impl<'a> Command<'a> { Command::Shortcut(r) => r.is_ok(), Command::Close(r) => r.is_ok(), Command::Note(r) => r.is_ok(), + Command::DecisionProcess(r) => r.is_ok(), } } diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs new file mode 100644 index 00000000..4981e6bf --- /dev/null +++ b/parser/src/command/decision.rs @@ -0,0 +1,188 @@ +//! The decision process command parser. +//! +//! This can parse arbitrary input, giving the user to be assigned. +//! +//! The grammar is as follows: +//! +//! ```text +//! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. +//! ``` + +use crate::error::Error; +use crate::token::{Token, Tokenizer}; +use std::fmt; + +/// A command as parsed and received from calling the bot with some arguments, +/// like `@rustbot merge` +pub struct DecisionCommand { + user: String, + disposition: Resolution, + reversibility: Reversibility, + issue_id: String, + comment_id: String, +} + + +#[derive(Debug)] +pub enum Error { + /// The first command that was given to this bot is not a valid one. + /// Decision process must start with a resolution. + InvalidFirstCommand, +} + +use Error::*; + +#[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)] +enum Reversibility { + Reversible, + Irreversible, +} + +use Reversibility::*; + +impl fmt::Display for Reversibility { + fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + match self { + Reversible => write!(formatter, "a **reversible**"), + Irreversible => writeln!(formatter, "an **irreversible**"), + } + } +} + +#[derive(Debug, PartialEq, Serialize, Deserialize)] +enum Resolution { + Hold, + Custom(String), +} + +use Resolution::*; + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct UserStatus { + name: String, + issue_id: String, + comment_id: String, +} + +impl UserStatus { + fn new(name: String, issue_id: String, comment_id: String) -> UserStatus { + UserStatus { + name, + issue_id, + comment_id, + } + } +} + +impl fmt::Display for UserStatus { + fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + write!(formatter, "{}", self.name) + } +} +pub trait LinkRenderer { + fn render_link(&self, data: &UserStatus) -> String; +} + +#[derive(Debug, Serialize, Deserialize)] +pub struct State { + initiator: String, + team_members: Vec, + period_start: DateTime, + original_period_start: DateTime, + current_statuses: HashMap, + status_history: HashMap>, + reversibility: Reversibility, + resolution: Resolution, +} + +impl State { + /// Renders the current state to the form it will have when seen as a + /// comment in the github issue/PR + pub fn render(&self, renderer: &impl LinkRenderer) -> String { + let initiator = &self.initiator; + let reversibility = self.reversibility.to_string(); + let comment = format!("Hello! {initiator} has proposed to merge this. This is {reversibility} decision, which means that it will be affirmed once the \"final comment period\" of 10 days have passed, unless a team member places a \"hold\" on the decision (or cancels it).\n\n"); + + let mut table = String::from(if self.status_history.is_empty() { + "| Team member | State |\n\ + |-------------|-------|\n" + } else { + "| Team member | History | State |\n\ + |-------------|---------|-------|\n" + }); + + for member in self.team_members.iter() { + let current_status = self + .current_statuses + .get(member) + .map(|status| { + let link = renderer.render_link(status); + + format!("[{status}]({link})") + }) + .unwrap_or_else(|| "".into()); + + if self.status_history.is_empty() { + table.push_str(&format!("| {member} | {current_status} |\n")); + } else { + let status_history = self + .status_history + .get(member) + .map(|statuses| { + statuses + .iter() + .map(|status| { + let link = renderer.render_link(status); + + format!("[{status}]({link})") + }) + .collect::>() + .join(" ") + }) + .unwrap_or_else(|| "".into()); + + table.push_str(&format!( + "| {member} | {status_history} | {current_status} |\n" + )); + } + } + + comment + &table + } +} + +impl Command { + #[cfg(test)] + fn merge(user: String, issue_id: String, comment_id: String) -> Self { + Self { + user, + issue_id, + comment_id, + disposition: Custom("merge".to_owned()), + reversibility: Reversibility::Reversible, + } + } + + #[cfg(test)] + fn hold(user: String, issue_id: String, comment_id: String) -> Self { + Self { + user, + issue_id, + comment_id, + disposition: Hold, + reversibility: Reversibility::Reversible, + } + } +} + + +pub struct Context { + team_members: Vec, + now: DateTime, +} + +impl Context { + pub fn new(team_members: Vec, now: DateTime) -> Context { + Context { team_members, now } + } +} diff --git a/src/handlers.rs b/src/handlers.rs index e479f75e..1257dcc5 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -44,6 +44,7 @@ mod review_submitted; mod rfc_helper; mod rustc_commits; mod shortcut; +mod decision; pub async fn handle(ctx: &Context, event: &Event) -> Vec { let config = config::get(&ctx.github, event.repo()).await; @@ -274,6 +275,7 @@ command_handlers! { shortcut: Shortcut, close: Close, note: Note, + decision: Decision, } pub struct Context { diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs new file mode 100644 index 00000000..608fa3e3 --- /dev/null +++ b/src/handlers/decision.rs @@ -0,0 +1,211 @@ +use parser::command::decision::{ + Context, DecisionCommand, Error, Resolution, Reversibility, State, UserStatus, InvalidFirstCommand +}; +use parser::command::decision::Resolution::*; +use std::collections::HashMap; + +/// Applies a command to the current state and returns the next state +pub fn handle_command( + state: Option, + command: DecisionCommand, + context: Context, +) -> Result { + let DecisionCommand { + user, + issue_id, + comment_id, + disposition, + reversibility, + } = command; + + if let Some(state) = state { + let name = match disposition { + Hold => "hold".into(), + Custom(name) => name, + }; + + let mut current_statuses = state.current_statuses; + let mut status_history = state.status_history; + + if let Some(entry) = current_statuses.get_mut(&user) { + let past = status_history.entry(user).or_insert(Vec::new()); + + past.push(entry.clone()); + + *entry = UserStatus::new(name, issue_id, comment_id); + } else { + current_statuses.insert(user, UserStatus::new("hold".into(), issue_id, comment_id)); + } + + Ok(State { + current_statuses, + status_history, + ..state + }) + } else { + // no state, this is the first call to the decision process + match disposition { + Hold => Err(InvalidFirstCommand), + + Custom(name) => { + let mut statuses = HashMap::new(); + + statuses.insert( + user.clone(), + UserStatus::new(name.clone(), issue_id, comment_id), + ); + + let Context { team_members, now } = context; + + Ok(State { + initiator: user, + team_members, + period_start: now, + original_period_start: now, + current_statuses: statuses, + status_history: HashMap::new(), + reversibility, + resolution: Custom(name), + }) + } + } + } +} + +#[cfg(test)] +mod tests { + use chrono::{Duration, Utc}; + use pretty_assertions::assert_eq; + + use super::*; + + struct TestRenderer {} + + impl LinkRenderer for TestRenderer { + fn render_link(&self, data: &UserStatus) -> String { + let issue_id = &data.issue_id; + let comment_id = &data.comment_id; + + format!("http://example.com/issue/{issue_id}#comment={comment_id}") + } + } + + /// Example 1 + /// + /// https://lang-team.rust-lang.org/decision_process/examples.html#reversible-decision-merging-a-proposal + /// + /// * From the starting point of there not being any state, someone proposes + /// to merge a proposal + /// * then barbara holds + /// * 11 days pass + /// * barbara says merge, it immediatly merges + #[test] + fn example_merging_proposal() { + let team_members = vec![ + "@Alan".to_owned(), + "@Barbara".to_owned(), + "@Grace".to_owned(), + "@Niklaus".to_owned(), + ]; + let r = TestRenderer {}; + + // alan proposes to merge + let time1 = Utc::now(); + let command = DecisionCommand::merge("@Alan".into(), "1".into(), "1".into()); + let state = handle_command(None, command, Context::new(team_members.clone(), time1)).unwrap(); + + assert_eq!(state.period_start, time1); + assert_eq!(state.original_period_start, time1); + assert_eq!( + state.current_statuses, + vec![( + "@Alan".into(), + UserStatus::new("merge".into(), "1".into(), "1".into()) + ),] + .into_iter() + .collect() + ); + assert_eq!(state.status_history, HashMap::new()); + assert_eq!(state.reversibility, Reversibility::Reversible); + assert_eq!(state.resolution, Custom("merge".into())); + assert_eq!( + state.render(&r), + include_str!("../../test/decision/res/01_merging_proposal__1.md") + ); + + // barbara holds + let time2 = Utc::now(); + let command = DecisionCommand::hold("@Barbara".into(), "1".into(), "2".into()); + let state = handle_command( + Some(state), + command, + Context::new(team_members.clone(), time2), + ) + .unwrap(); + + assert_eq!(state.period_start, time1); + assert_eq!(state.original_period_start, time1); + assert_eq!( + state.current_statuses, + vec![ + ( + "@Alan".into(), + UserStatus::new("merge".into(), "1".into(), "1".into()) + ), + ( + "@Barbara".into(), + UserStatus::new("hold".into(), "1".into(), "2".into()) + ), + ] + .into_iter() + .collect() + ); + assert_eq!(state.status_history, HashMap::new()); + assert_eq!(state.reversibility, Reversibility::Reversible); + assert_eq!(state.resolution, Custom("merge".into())); + assert_eq!( + state.render(&r), + include_str!("../../test/decision/res/01_merging_proposal__2.md") + ); + + // 11 days pass + let time3 = time2 + Duration::days(11); + + // Barbara says merge, it immediatly merges + let command = DecisionCommand::merge("@Barbara".into(), "1".into(), "3".into()); + let state = handle_command(Some(state), command, Context::new(team_members, time3)).unwrap(); + + assert_eq!(state.period_start, time1); + assert_eq!(state.original_period_start, time1); + assert_eq!( + state.current_statuses, + vec![ + ( + "@Alan".into(), + UserStatus::new("merge".into(), "1".into(), "1".into()) + ), + ( + "@Barbara".into(), + UserStatus::new("merge".into(), "1".into(), "3".into()) + ), + ] + .into_iter() + .collect() + ); + assert_eq!( + state.status_history, + vec![( + "@Barbara".into(), + vec![UserStatus::new("hold".into(), "1".into(), "2".into())] + ),] + .into_iter() + .collect() + ); + assert_eq!(state.reversibility, Reversibility::Reversible); + assert_eq!(state.resolution, Custom("merge".into())); + assert_eq!( + state.render(&r), + include_str!("../../test/decision/01_merging_proposal__3.md") + ); + } +} diff --git a/test/decision/01_merging_proposal__1.md b/test/decision/01_merging_proposal__1.md new file mode 100644 index 00000000..f77ea4e1 --- /dev/null +++ b/test/decision/01_merging_proposal__1.md @@ -0,0 +1,8 @@ +Hello! @Alan has proposed to merge this. This is a **reversible** decision, which means that it will be affirmed once the "final comment period" of 10 days have passed, unless a team member places a "hold" on the decision (or cancels it). + +| Team member | State | +|-------------|-------| +| @Alan | [merge](http://example.com/issue/1#comment=1) | +| @Barbara | | +| @Grace | | +| @Niklaus | | diff --git a/test/decision/01_merging_proposal__2.md b/test/decision/01_merging_proposal__2.md new file mode 100644 index 00000000..ae2225f0 --- /dev/null +++ b/test/decision/01_merging_proposal__2.md @@ -0,0 +1,8 @@ +Hello! @Alan has proposed to merge this. This is a **reversible** decision, which means that it will be affirmed once the "final comment period" of 10 days have passed, unless a team member places a "hold" on the decision (or cancels it). + +| Team member | State | +|-------------|-------| +| @Alan | [merge](http://example.com/issue/1#comment=1) | +| @Barbara | [hold](http://example.com/issue/1#comment=2) | +| @Grace | | +| @Niklaus | | diff --git a/test/decision/01_merging_proposal__3.md b/test/decision/01_merging_proposal__3.md new file mode 100644 index 00000000..dd8676c9 --- /dev/null +++ b/test/decision/01_merging_proposal__3.md @@ -0,0 +1,8 @@ +Hello! @Alan has proposed to merge this. This is a **reversible** decision, which means that it will be affirmed once the "final comment period" of 10 days have passed, unless a team member places a "hold" on the decision (or cancels it). + +| Team member | History | State | +|-------------|---------|-------| +| @Alan | | [merge](http://example.com/issue/1#comment=1) | +| @Barbara | [hold](http://example.com/issue/1#comment=2) | [merge](http://example.com/issue/1#comment=3) | +| @Grace | | | +| @Niklaus | | | From 4eab2b53e6cfbb1c50be1ac6094a3f17832be271 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 14 Sep 2022 15:24:17 -0300 Subject: [PATCH 02/15] Some cleaning --- .idea/workspace.xml | 90 ---------------------------------- parser/src/command.rs | 7 ++- parser/src/command/decision.rs | 1 + src/config.rs | 10 ++++ src/handlers/decision.rs | 9 ++-- 5 files changed, 22 insertions(+), 95 deletions(-) delete mode 100644 .idea/workspace.xml diff --git a/.idea/workspace.xml b/.idea/workspace.xml deleted file mode 100644 index d52ebb7f..00000000 --- a/.idea/workspace.xml +++ /dev/null @@ -1,90 +0,0 @@ - - - - - { - "useNewFormat": true -} - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 1661637398588 - - - - - - - - - \ No newline at end of file diff --git a/parser/src/command.rs b/parser/src/command.rs index fbaa5dd2..94fe3d54 100644 --- a/parser/src/command.rs +++ b/parser/src/command.rs @@ -134,6 +134,11 @@ impl<'a> Input<'a> { Command::Close, &original_tokenizer, )); + success.extend(parse_single_command( + decision::DecisionCommand::parse, + Command::Decision, + &original_tokenizer, + )); if success.len() > 1 { panic!( @@ -209,7 +214,7 @@ impl<'a> Command<'a> { Command::Shortcut(r) => r.is_ok(), Command::Close(r) => r.is_ok(), Command::Note(r) => r.is_ok(), - Command::DecisionProcess(r) => r.is_ok(), + Command::Decision(r) => r.is_ok(), } } diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 4981e6bf..5881dc3a 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -14,6 +14,7 @@ use std::fmt; /// A command as parsed and received from calling the bot with some arguments, /// like `@rustbot merge` +#[derive(PartialEq, Eq, Debug)] pub struct DecisionCommand { user: String, disposition: Resolution, diff --git a/src/config.rs b/src/config.rs index 053aa682..203d2366 100644 --- a/src/config.rs +++ b/src/config.rs @@ -34,6 +34,7 @@ pub(crate) struct Config { pub(crate) note: Option, pub(crate) mentions: Option, pub(crate) no_merges: Option, + pub(crate) decision: Option, } #[derive(PartialEq, Eq, Debug, serde::Deserialize)] @@ -257,6 +258,12 @@ pub(crate) struct GitHubReleasesConfig { pub(crate) changelog_branch: String, } +#[derive(PartialEq, Eq, Debug, serde::Deserialize)] +pub(crate) struct DecisionConfig { + #[serde(default)] + _empty: (), +} + fn get_cached_config(repo: &str) -> Option, ConfigurationError>> { let cache = CONFIG_CACHE.read().unwrap(); cache.get(repo).and_then(|(config, fetch_time)| { @@ -343,6 +350,8 @@ mod tests { infra = "T-infra" [shortcut] + + [decision-process] "#; let config = toml::from_str::(&config).unwrap(); let mut ping_teams = HashMap::new(); @@ -395,6 +404,7 @@ mod tests { review_submitted: None, mentions: None, no_merges: None, + decision: None } ); } diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 608fa3e3..6e7442a8 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -5,10 +5,11 @@ use parser::command::decision::Resolution::*; use std::collections::HashMap; /// Applies a command to the current state and returns the next state -pub fn handle_command( - state: Option, - command: DecisionCommand, - context: Context, +pub(super) async fn handle_command( + ctx: &Context, + _config: &DecisionConfig, + event: &Event, + cmd: DecisionCommand, ) -> Result { let DecisionCommand { user, From 8243bd3a91cd39a0d52fabb20ba4b5071c5760d5 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 19 Oct 2022 17:03:20 -0300 Subject: [PATCH 03/15] Add decision state db support --- src/db.rs | 18 +++++++ src/db/decision_state.rs | 108 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 src/db/decision_state.rs diff --git a/src/db.rs b/src/db.rs index 8b30e4c4..61336795 100644 --- a/src/db.rs +++ b/src/db.rs @@ -8,6 +8,7 @@ use std::sync::{Arc, Mutex}; use tokio::sync::{OwnedSemaphorePermit, Semaphore}; use tokio_postgres::Client as DbClient; +pub mod decision_state; pub mod issue_data; pub mod jobs; pub mod notifications; @@ -273,4 +274,21 @@ CREATE UNIQUE INDEX jobs_name_scheduled_at_unique_index name, scheduled_at ); ", + " +CREATE TYPE reversibility AS ENUM ('reversible', 'irreversible'); +", + " +CREATE TYPE resolution AS ENUM ('hold', 'merge'); +", + "CREATE TABLE decision_state ( + issue_id BIGINT PRIMARY KEY, + initiator TEXT NOT NULL, + team_members JSONB NOT NULL, + start_date TIMESTAMP WITH TIME ZONE NOT NULL, + period_end_date TIMESTAMP WITH TIME ZONE NOT NULL, + current_statuses JSONB NOT NULL, + status_history JSONB, + reversibility reversibility NOT NULL DEFAULT 'reversible', + resolution resolution NOT NULL DEFAULT 'merge' +);", ]; diff --git a/src/db/decision_state.rs b/src/db/decision_state.rs new file mode 100644 index 00000000..cadf6b59 --- /dev/null +++ b/src/db/decision_state.rs @@ -0,0 +1,108 @@ +//! The decision state table provides a way to store the state of each issue + +use serde::{Deserialize, Serialize}; +use chrono::{DateTime, FixedOffset}; +use std::collections::HashMap; +use parser::command::decision::{Resolution, Reversibility}; +use anyhow::{Context as _, Result}; +use tokio_postgres::Client as DbClient; + +#[derive(Debug, Serialize, Deserialize)] +pub struct DecisionState { + issue_id: String, + initiator: String, + team_members: Vec, + start_date: DateTime, + period_date: DateTime, + current_statuses: HashMap, + status_history: HashMap>, + reversibility: Reversibility, + resolution: Resolution, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct UserStatus { + name: String, + issue_id: String, + comment_id: String, +} + +pub async fn insert_decision_state( + db: &DbClient, + issue_id: &String, + initiator: &String, + team_members: &Vec, + start_date: &DateTime, + period_end_date: &DateTime, + current_statuses: &HashMap, + status_history: &HashMap>, + reversibility: &Reversibility, + resolution: &Resolution, +) -> Result<()> { + tracing::trace!("insert_decision_state(issue_id={})", issue_id); + + db.execute( + "INSERT INTO decision_state (issue_id, initiator, team_members, start_date, period_end_date, current_statuses, status_history, reversibility, resolution) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) + ON CONFLICT issue_id DO NOTHING", + &[&issue_id, &initiator, &team_members, &start_date, &period_end_date, ¤t_statuses, &status_history, &reversibility, &resolution], + ) + .await + .context("Inserting decision state")?; + + Ok(()) +} + +// pub async fn update_decision_state( +// db: &DbClient, +// issue_id: &String, +// period_end_date: &DateTime, +// current_statuses: &HashMap, +// status_history: &HashMap>, +// reversibility: &Reversibility, +// resolution: &Resolution +// ) -> Result<()> { +// tracing::trace!("update_decision_state(issue_id={})", issue_id); + +// db.execute("UPDATE decision_state SET period_end_date = $2, current_statuses = $3, status_history = $4, reversibility = $5, resolution = $6 WHERE issue_id = $1", +// &[&issue_id, &period_end_date, ¤t_statuses, &status_history, &reversibility, &resolution] +// ) +// .await +// .context("Updating decision state")?; + +// Ok(()) +// } + +// pub async fn get_decision_state_for_issue(db: &DbClient, issue_id: &String) -> Result { +// let state = db +// .query( +// "SELECT * FROM decision_state WHERE issue_id = $1", +// &[&issue_id] +// ) +// .await +// .context("Getting decision state data")?; + + +// let issue_id: String = state.get(0); +// let initiator: String = state.get(1); +// let team_members: Vec = state.get(2); +// let start_date: DateTime = state.get(3); +// let period_date: DateTime = state.get(4); +// let current_statuses: HashMap = state.get(5); +// let status_history: HashMap> = state.get(6); +// let reversibility: Reversibility = state.get(7); +// let resolution: Resolution = state.get(8); + +// Ok( +// DecisionState { +// issue_id, +// initiator, +// team_members, +// start_date, +// period_date, +// current_statuses, +// status_history, +// reversibility, +// resolution, +// } +// ) +// } From fdc6255c51f453fb3c1910d701476598440e6b97 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 19 Oct 2022 17:03:27 -0300 Subject: [PATCH 04/15] Start working on handler - wip --- Cargo.lock | 6 +- parser/Cargo.toml | 5 + parser/src/command/decision.rs | 191 +++------------ src/db/decision_state.rs | 10 +- src/handlers/decision.rs | 425 ++++++++++++++++++--------------- 5 files changed, 269 insertions(+), 368 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ffdfcb71..6ddeb796 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1237,8 +1237,10 @@ name = "parser" version = "0.1.0" dependencies = [ "log", + "postgres-types", "pulldown-cmark", "regex", + "serde", ] [[package]] @@ -1348,9 +1350,9 @@ checksum = "1df8c4ec4b0627e53bdf214615ad287367e482558cf84b109250b37464dc03ae" [[package]] name = "postgres-derive" -version = "0.4.2" +version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d0c2c18e40b92144b05e6f3ae9d1ee931f0d1afa9410ac8b97486c6eaaf91201" +checksum = "9e76c801e97c9cf696097369e517785b98056e98b21149384c812febfc5912f2" dependencies = [ "proc-macro2", "quote", diff --git a/parser/Cargo.toml b/parser/Cargo.toml index 22bf0aaf..d73d4aa9 100644 --- a/parser/Cargo.toml +++ b/parser/Cargo.toml @@ -8,3 +8,8 @@ edition = "2021" pulldown-cmark = "0.7.0" log = "0.4" regex = "1.6.0" +postgres-types = { version = "0.2.4", features = ["derive"] } + +[dependencies.serde] +version = "1" +features = ["derive"] diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 5881dc3a..31e22c97 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -8,182 +8,47 @@ //! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. //! ``` +use crate::token::{Tokenizer}; use crate::error::Error; -use crate::token::{Token, Tokenizer}; -use std::fmt; +use serde::{Deserialize, Serialize}; +use postgres_types::{FromSql, ToSql}; /// A command as parsed and received from calling the bot with some arguments, /// like `@rustbot merge` -#[derive(PartialEq, Eq, Debug)] +#[derive(Debug, Eq, PartialEq)] pub struct DecisionCommand { - user: String, - disposition: Resolution, - reversibility: Reversibility, - issue_id: String, - comment_id: String, + pub resolution: Resolution, + pub reversibility: Reversibility } - -#[derive(Debug)] -pub enum Error { - /// The first command that was given to this bot is not a valid one. - /// Decision process must start with a resolution. - InvalidFirstCommand, +impl DecisionCommand { + pub fn parse<'a>(_input: &mut Tokenizer<'a>) -> Result, Error<'a>> { + Ok(Some(Self { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible + })) + } } -use Error::*; +#[derive(Debug, Eq, PartialEq)] +pub enum ParseError { + InvalidFirstCommand +} -#[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)] -enum Reversibility { +#[derive(Serialize, Deserialize, Debug, ToSql, FromSql, Eq, PartialEq)] +#[postgres(name = "reversibility")] +pub enum Reversibility { + #[postgres(name = "reversible")] Reversible, + #[postgres(name = "irreversible")] Irreversible, } -use Reversibility::*; - -impl fmt::Display for Reversibility { - fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - match self { - Reversible => write!(formatter, "a **reversible**"), - Irreversible => writeln!(formatter, "an **irreversible**"), - } - } -} - -#[derive(Debug, PartialEq, Serialize, Deserialize)] -enum Resolution { +#[derive(Serialize, Deserialize, Debug, ToSql, FromSql, Eq, PartialEq)] +#[postgres(name = "resolution")] +pub enum Resolution { + #[postgres(name = "merge")] + Merge, + #[postgres(name = "hold")] Hold, - Custom(String), -} - -use Resolution::*; - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct UserStatus { - name: String, - issue_id: String, - comment_id: String, -} - -impl UserStatus { - fn new(name: String, issue_id: String, comment_id: String) -> UserStatus { - UserStatus { - name, - issue_id, - comment_id, - } - } -} - -impl fmt::Display for UserStatus { - fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - write!(formatter, "{}", self.name) - } -} -pub trait LinkRenderer { - fn render_link(&self, data: &UserStatus) -> String; -} - -#[derive(Debug, Serialize, Deserialize)] -pub struct State { - initiator: String, - team_members: Vec, - period_start: DateTime, - original_period_start: DateTime, - current_statuses: HashMap, - status_history: HashMap>, - reversibility: Reversibility, - resolution: Resolution, -} - -impl State { - /// Renders the current state to the form it will have when seen as a - /// comment in the github issue/PR - pub fn render(&self, renderer: &impl LinkRenderer) -> String { - let initiator = &self.initiator; - let reversibility = self.reversibility.to_string(); - let comment = format!("Hello! {initiator} has proposed to merge this. This is {reversibility} decision, which means that it will be affirmed once the \"final comment period\" of 10 days have passed, unless a team member places a \"hold\" on the decision (or cancels it).\n\n"); - - let mut table = String::from(if self.status_history.is_empty() { - "| Team member | State |\n\ - |-------------|-------|\n" - } else { - "| Team member | History | State |\n\ - |-------------|---------|-------|\n" - }); - - for member in self.team_members.iter() { - let current_status = self - .current_statuses - .get(member) - .map(|status| { - let link = renderer.render_link(status); - - format!("[{status}]({link})") - }) - .unwrap_or_else(|| "".into()); - - if self.status_history.is_empty() { - table.push_str(&format!("| {member} | {current_status} |\n")); - } else { - let status_history = self - .status_history - .get(member) - .map(|statuses| { - statuses - .iter() - .map(|status| { - let link = renderer.render_link(status); - - format!("[{status}]({link})") - }) - .collect::>() - .join(" ") - }) - .unwrap_or_else(|| "".into()); - - table.push_str(&format!( - "| {member} | {status_history} | {current_status} |\n" - )); - } - } - - comment + &table - } -} - -impl Command { - #[cfg(test)] - fn merge(user: String, issue_id: String, comment_id: String) -> Self { - Self { - user, - issue_id, - comment_id, - disposition: Custom("merge".to_owned()), - reversibility: Reversibility::Reversible, - } - } - - #[cfg(test)] - fn hold(user: String, issue_id: String, comment_id: String) -> Self { - Self { - user, - issue_id, - comment_id, - disposition: Hold, - reversibility: Reversibility::Reversible, - } - } -} - - -pub struct Context { - team_members: Vec, - now: DateTime, -} - -impl Context { - pub fn new(team_members: Vec, now: DateTime) -> Context { - Context { team_members, now } - } } diff --git a/src/db/decision_state.rs b/src/db/decision_state.rs index cadf6b59..28502451 100644 --- a/src/db/decision_state.rs +++ b/src/db/decision_state.rs @@ -11,9 +11,8 @@ use tokio_postgres::Client as DbClient; pub struct DecisionState { issue_id: String, initiator: String, - team_members: Vec, start_date: DateTime, - period_date: DateTime, + end_date: DateTime, current_statuses: HashMap, status_history: HashMap>, reversibility: Reversibility, @@ -31,9 +30,8 @@ pub async fn insert_decision_state( db: &DbClient, issue_id: &String, initiator: &String, - team_members: &Vec, start_date: &DateTime, - period_end_date: &DateTime, + end_date: &DateTime, current_statuses: &HashMap, status_history: &HashMap>, reversibility: &Reversibility, @@ -42,9 +40,9 @@ pub async fn insert_decision_state( tracing::trace!("insert_decision_state(issue_id={})", issue_id); db.execute( - "INSERT INTO decision_state (issue_id, initiator, team_members, start_date, period_end_date, current_statuses, status_history, reversibility, resolution) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) + "INSERT INTO decision_state (issue_id, initiator, start_date, end_date, current_statuses, status_history, reversibility, resolution) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) ON CONFLICT issue_id DO NOTHING", - &[&issue_id, &initiator, &team_members, &start_date, &period_end_date, ¤t_statuses, &status_history, &reversibility, &resolution], + &[&issue_id, &initiator, &start_date, &end_date, ¤t_statuses, &status_history, &reversibility, &resolution], ) .await .context("Inserting decision state")?; diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 6e7442a8..6dfa27c7 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,212 +1,243 @@ -use parser::command::decision::{ - Context, DecisionCommand, Error, Resolution, Reversibility, State, UserStatus, InvalidFirstCommand +use parser::command::decision::{DecisionCommand, ParseError}; +use crate::{ + config::DecisionConfig, + github::{self, Event}, + handlers::Context, + interactions::ErrorComment, + db::decision_state::* }; -use parser::command::decision::Resolution::*; use std::collections::HashMap; +use chrono::{DateTime, Duration, Utc}; + +// get state for issue_id from db +// if no state (first call) + // initialize state + // schedule job if necessary + // send comment to github + // save state +// if state + // apply logic to decide what to do + // schedule job if necessary + // send comment to github + // save state -/// Applies a command to the current state and returns the next state pub(super) async fn handle_command( ctx: &Context, _config: &DecisionConfig, event: &Event, cmd: DecisionCommand, -) -> Result { +) -> anyhow::Result<()> { let DecisionCommand { - user, - issue_id, - comment_id, - disposition, - reversibility, - } = command; - - if let Some(state) = state { - let name = match disposition { - Hold => "hold".into(), - Custom(name) => name, - }; - - let mut current_statuses = state.current_statuses; - let mut status_history = state.status_history; - - if let Some(entry) = current_statuses.get_mut(&user) { - let past = status_history.entry(user).or_insert(Vec::new()); - - past.push(entry.clone()); - - *entry = UserStatus::new(name, issue_id, comment_id); - } else { - current_statuses.insert(user, UserStatus::new("hold".into(), issue_id, comment_id)); - } + resolution, + reversibility + } = cmd; + + let issue = event.issue().unwrap(); + + let is_team_member = event + .user() + .is_team_member(&ctx.github) + .await + .unwrap_or(false); + + if !is_team_member { + let cmnt = ErrorComment::new(&issue, "Only team members can be part of the decision process."); + cmnt.post(&ctx.github).await?; + return Ok(()); + } - Ok(State { - current_statuses, - status_history, - ..state - }) - } else { - // no state, this is the first call to the decision process - match disposition { - Hold => Err(InvalidFirstCommand), - - Custom(name) => { - let mut statuses = HashMap::new(); - - statuses.insert( - user.clone(), - UserStatus::new(name.clone(), issue_id, comment_id), - ); - - let Context { team_members, now } = context; - - Ok(State { - initiator: user, - team_members, - period_start: now, - original_period_start: now, - current_statuses: statuses, - status_history: HashMap::new(), - reversibility, - resolution: Custom(name), - }) + // return Ok(()); + + match get_decision_state(issue.id) { + Some(state) => { + // let name = match disposition { + // Hold => "hold".into(), + // Custom(name) => name, + // }; + + // let mut current_statuses = state.current_statuses; + // let mut status_history = state.status_history; + + // if let Some(entry) = current_statuses.get_mut(&user) { + // let past = status_history.entry(user).or_insert(Vec::new()); + + // past.push(entry.clone()); + + // *entry = UserStatus::new(name, issue_id, comment_id); + // } else { + // current_statuses.insert(user, UserStatus::new("hold".into(), issue_id, comment_id)); + // } + + // Ok(State { + // current_statuses, + // status_history, + // ..state + // }) + Ok() + }, + None => { + match resolution { + Hold => Err(ParseError::InvalidFirstCommand), + Merge => { + let start_date = chrono::Utc::now().into(); + let end_date = start_date.checked_add_signed(Duration::days(10)).unwrap(); + + let current_statuses = HashMap::new(); + let status_history = HashMap::new(); + + let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? + + insert_decision_state( + db, + issue.id, + user.login, + start_date, + end_date, + current_statuses, + status_history, + reversibility, + resolution, + ); + } } } } } -#[cfg(test)] -mod tests { - use chrono::{Duration, Utc}; - use pretty_assertions::assert_eq; - - use super::*; - - struct TestRenderer {} - - impl LinkRenderer for TestRenderer { - fn render_link(&self, data: &UserStatus) -> String { - let issue_id = &data.issue_id; - let comment_id = &data.comment_id; - - format!("http://example.com/issue/{issue_id}#comment={comment_id}") - } - } - - /// Example 1 - /// - /// https://lang-team.rust-lang.org/decision_process/examples.html#reversible-decision-merging-a-proposal - /// - /// * From the starting point of there not being any state, someone proposes - /// to merge a proposal - /// * then barbara holds - /// * 11 days pass - /// * barbara says merge, it immediatly merges - #[test] - fn example_merging_proposal() { - let team_members = vec![ - "@Alan".to_owned(), - "@Barbara".to_owned(), - "@Grace".to_owned(), - "@Niklaus".to_owned(), - ]; - let r = TestRenderer {}; - - // alan proposes to merge - let time1 = Utc::now(); - let command = DecisionCommand::merge("@Alan".into(), "1".into(), "1".into()); - let state = handle_command(None, command, Context::new(team_members.clone(), time1)).unwrap(); - - assert_eq!(state.period_start, time1); - assert_eq!(state.original_period_start, time1); - assert_eq!( - state.current_statuses, - vec![( - "@Alan".into(), - UserStatus::new("merge".into(), "1".into(), "1".into()) - ),] - .into_iter() - .collect() - ); - assert_eq!(state.status_history, HashMap::new()); - assert_eq!(state.reversibility, Reversibility::Reversible); - assert_eq!(state.resolution, Custom("merge".into())); - assert_eq!( - state.render(&r), - include_str!("../../test/decision/res/01_merging_proposal__1.md") - ); - - // barbara holds - let time2 = Utc::now(); - let command = DecisionCommand::hold("@Barbara".into(), "1".into(), "2".into()); - let state = handle_command( - Some(state), - command, - Context::new(team_members.clone(), time2), - ) - .unwrap(); - - assert_eq!(state.period_start, time1); - assert_eq!(state.original_period_start, time1); - assert_eq!( - state.current_statuses, - vec![ - ( - "@Alan".into(), - UserStatus::new("merge".into(), "1".into(), "1".into()) - ), - ( - "@Barbara".into(), - UserStatus::new("hold".into(), "1".into(), "2".into()) - ), - ] - .into_iter() - .collect() - ); - assert_eq!(state.status_history, HashMap::new()); - assert_eq!(state.reversibility, Reversibility::Reversible); - assert_eq!(state.resolution, Custom("merge".into())); - assert_eq!( - state.render(&r), - include_str!("../../test/decision/res/01_merging_proposal__2.md") - ); - - // 11 days pass - let time3 = time2 + Duration::days(11); - - // Barbara says merge, it immediatly merges - let command = DecisionCommand::merge("@Barbara".into(), "1".into(), "3".into()); - let state = handle_command(Some(state), command, Context::new(team_members, time3)).unwrap(); - - assert_eq!(state.period_start, time1); - assert_eq!(state.original_period_start, time1); - assert_eq!( - state.current_statuses, - vec![ - ( - "@Alan".into(), - UserStatus::new("merge".into(), "1".into(), "1".into()) - ), - ( - "@Barbara".into(), - UserStatus::new("merge".into(), "1".into(), "3".into()) - ), - ] - .into_iter() - .collect() - ); - assert_eq!( - state.status_history, - vec![( - "@Barbara".into(), - vec![UserStatus::new("hold".into(), "1".into(), "2".into())] - ),] - .into_iter() - .collect() - ); - assert_eq!(state.reversibility, Reversibility::Reversible); - assert_eq!(state.resolution, Custom("merge".into())); - assert_eq!( - state.render(&r), - include_str!("../../test/decision/01_merging_proposal__3.md") - ); - } -} +// #[cfg(test)] +// mod tests { +// use chrono::{Duration, Utc}; +// use pretty_assertions::assert_eq; + +// use super::*; + +// struct TestRenderer {} + +// impl LinkRenderer for TestRenderer { +// fn render_link(&self, data: &UserStatus) -> String { +// let issue_id = &data.issue_id; +// let comment_id = &data.comment_id; + +// format!("http://example.com/issue/{issue_id}#comment={comment_id}") +// } +// } + +// /// Example 1 +// /// +// /// https://lang-team.rust-lang.org/decision_process/examples.html#reversible-decision-merging-a-proposal +// /// +// /// * From the starting point of there not being any state, someone proposes +// /// to merge a proposal +// /// * then barbara holds +// /// * 11 days pass +// /// * barbara says merge, it immediatly merges +// #[test] +// fn example_merging_proposal() { +// let team_members = vec![ +// "@Alan".to_owned(), +// "@Barbara".to_owned(), +// "@Grace".to_owned(), +// "@Niklaus".to_owned(), +// ]; +// let r = TestRenderer {}; + +// // alan proposes to merge +// let time1 = Utc::now(); +// let command = DecisionCommand::merge("@Alan".into(), "1".into(), "1".into()); +// let state = handle_command(None, command, Context::new(team_members.clone(), time1)).unwrap(); + +// assert_eq!(state.period_start, time1); +// assert_eq!(state.original_period_start, time1); +// assert_eq!( +// state.current_statuses, +// vec![( +// "@Alan".into(), +// UserStatus::new("merge".into(), "1".into(), "1".into()) +// ),] +// .into_iter() +// .collect() +// ); +// assert_eq!(state.status_history, HashMap::new()); +// assert_eq!(state.reversibility, Reversibility::Reversible); +// assert_eq!(state.resolution, Custom("merge".into())); +// assert_eq!( +// state.render(&r), +// include_str!("../../test/decision/res/01_merging_proposal__1.md") +// ); + +// // barbara holds +// let time2 = Utc::now(); +// let command = DecisionCommand::hold("@Barbara".into(), "1".into(), "2".into()); +// let state = handle_command( +// Some(state), +// command, +// Context::new(team_members.clone(), time2), +// ) +// .unwrap(); + +// assert_eq!(state.period_start, time1); +// assert_eq!(state.original_period_start, time1); +// assert_eq!( +// state.current_statuses, +// vec![ +// ( +// "@Alan".into(), +// UserStatus::new("merge".into(), "1".into(), "1".into()) +// ), +// ( +// "@Barbara".into(), +// UserStatus::new("hold".into(), "1".into(), "2".into()) +// ), +// ] +// .into_iter() +// .collect() +// ); +// assert_eq!(state.status_history, HashMap::new()); +// assert_eq!(state.reversibility, Reversibility::Reversible); +// assert_eq!(state.resolution, Custom("merge".into())); +// assert_eq!( +// state.render(&r), +// include_str!("../../test/decision/res/01_merging_proposal__2.md") +// ); + +// // 11 days pass +// let time3 = time2 + Duration::days(11); + +// // Barbara says merge, it immediatly merges +// let command = DecisionCommand::merge("@Barbara".into(), "1".into(), "3".into()); +// let state = handle_command(Some(state), command, Context::new(team_members, time3)).unwrap(); + +// assert_eq!(state.period_start, time1); +// assert_eq!(state.original_period_start, time1); +// assert_eq!( +// state.current_statuses, +// vec![ +// ( +// "@Alan".into(), +// UserStatus::new("merge".into(), "1".into(), "1".into()) +// ), +// ( +// "@Barbara".into(), +// UserStatus::new("merge".into(), "1".into(), "3".into()) +// ), +// ] +// .into_iter() +// .collect() +// ); +// assert_eq!( +// state.status_history, +// vec![( +// "@Barbara".into(), +// vec![UserStatus::new("hold".into(), "1".into(), "2".into())] +// ),] +// .into_iter() +// .collect() +// ); +// assert_eq!(state.reversibility, Reversibility::Reversible); +// assert_eq!(state.resolution, Custom("merge".into())); +// assert_eq!( +// state.render(&r), +// include_str!("../../test/decision/01_merging_proposal__3.md") +// ); +// } +// } From 1a33f6c54ef2fcd6f3797de635aa6ef896180c37 Mon Sep 17 00:00:00 2001 From: Julian Montes de Oca Date: Wed, 26 Oct 2022 15:18:44 -0300 Subject: [PATCH 05/15] fix parse to test integration --- parser/src/command/decision.rs | 9 ++++++--- src/handlers/decision.rs | 19 ++++++++++++++++--- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 31e22c97..c6a6df18 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -8,7 +8,7 @@ //! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. //! ``` -use crate::token::{Tokenizer}; +use crate::token::{Token, Tokenizer}; use crate::error::Error; use serde::{Deserialize, Serialize}; use postgres_types::{FromSql, ToSql}; @@ -22,11 +22,14 @@ pub struct DecisionCommand { } impl DecisionCommand { - pub fn parse<'a>(_input: &mut Tokenizer<'a>) -> Result, Error<'a>> { + pub fn parse<'a>(input: &mut Tokenizer<'a>) -> Result, Error<'a>> { + if let Some(Token::Word("merge")) = input.peek_token()? { Ok(Some(Self { resolution: Resolution::Merge, reversibility: Reversibility::Reversible - })) + })) } else { + Ok(None) + } } } diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 6dfa27c7..3644c3cf 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,3 +1,4 @@ +use anyhow::Context as Ctx; use parser::command::decision::{DecisionCommand, ParseError}; use crate::{ config::DecisionConfig, @@ -46,8 +47,6 @@ pub(super) async fn handle_command( return Ok(()); } - // return Ok(()); - match get_decision_state(issue.id) { Some(state) => { // let name = match disposition { @@ -73,7 +72,7 @@ pub(super) async fn handle_command( // status_history, // ..state // }) - Ok() + Ok(); }, None => { match resolution { @@ -98,6 +97,20 @@ pub(super) async fn handle_command( reversibility, resolution, ); + + let comment = format!( + "Wow, it looks like you want to merge this, {}.", event.user().login + ); + + let comment = format!( + "| Team member | State |\n|-------------|-------|\n| julmontesdeoca | merge |\n| mcass19 | |"); + + issue + .post_comment(&ctx.github, &comment) + .await + .context("merge vote comment")?; + + Ok(); } } } From 328e025eb926974519f4a370cacaf59e7b9105a2 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 9 Nov 2022 16:00:15 -0300 Subject: [PATCH 06/15] Add db support for issue_decision_state --- parser/src/command/decision.rs | 4 +- src/db.rs | 11 ++-- src/db/decision_state.rs | 106 ------------------------------- src/db/issue_decision_state.rs | 112 +++++++++++++++++++++++++++++++++ src/handlers/decision.rs | 76 ++++++++++++---------- 5 files changed, 161 insertions(+), 148 deletions(-) delete mode 100644 src/db/decision_state.rs create mode 100644 src/db/issue_decision_state.rs diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index c6a6df18..1d24829d 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -38,7 +38,7 @@ pub enum ParseError { InvalidFirstCommand } -#[derive(Serialize, Deserialize, Debug, ToSql, FromSql, Eq, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] #[postgres(name = "reversibility")] pub enum Reversibility { #[postgres(name = "reversible")] @@ -47,7 +47,7 @@ pub enum Reversibility { Irreversible, } -#[derive(Serialize, Deserialize, Debug, ToSql, FromSql, Eq, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] #[postgres(name = "resolution")] pub enum Resolution { #[postgres(name = "merge")] diff --git a/src/db.rs b/src/db.rs index 61336795..8e82f866 100644 --- a/src/db.rs +++ b/src/db.rs @@ -8,7 +8,7 @@ use std::sync::{Arc, Mutex}; use tokio::sync::{OwnedSemaphorePermit, Semaphore}; use tokio_postgres::Client as DbClient; -pub mod decision_state; +pub mod issue_decision_state; pub mod issue_data; pub mod jobs; pub mod notifications; @@ -280,14 +280,13 @@ CREATE TYPE reversibility AS ENUM ('reversible', 'irreversible'); " CREATE TYPE resolution AS ENUM ('hold', 'merge'); ", - "CREATE TABLE decision_state ( +"CREATE TABLE issue_decision_state ( issue_id BIGINT PRIMARY KEY, initiator TEXT NOT NULL, - team_members JSONB NOT NULL, start_date TIMESTAMP WITH TIME ZONE NOT NULL, - period_end_date TIMESTAMP WITH TIME ZONE NOT NULL, - current_statuses JSONB NOT NULL, - status_history JSONB, + end_date TIMESTAMP WITH TIME ZONE NOT NULL, + current JSONB NOT NULL, + history JSONB, reversibility reversibility NOT NULL DEFAULT 'reversible', resolution resolution NOT NULL DEFAULT 'merge' );", diff --git a/src/db/decision_state.rs b/src/db/decision_state.rs deleted file mode 100644 index 28502451..00000000 --- a/src/db/decision_state.rs +++ /dev/null @@ -1,106 +0,0 @@ -//! The decision state table provides a way to store the state of each issue - -use serde::{Deserialize, Serialize}; -use chrono::{DateTime, FixedOffset}; -use std::collections::HashMap; -use parser::command::decision::{Resolution, Reversibility}; -use anyhow::{Context as _, Result}; -use tokio_postgres::Client as DbClient; - -#[derive(Debug, Serialize, Deserialize)] -pub struct DecisionState { - issue_id: String, - initiator: String, - start_date: DateTime, - end_date: DateTime, - current_statuses: HashMap, - status_history: HashMap>, - reversibility: Reversibility, - resolution: Resolution, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct UserStatus { - name: String, - issue_id: String, - comment_id: String, -} - -pub async fn insert_decision_state( - db: &DbClient, - issue_id: &String, - initiator: &String, - start_date: &DateTime, - end_date: &DateTime, - current_statuses: &HashMap, - status_history: &HashMap>, - reversibility: &Reversibility, - resolution: &Resolution, -) -> Result<()> { - tracing::trace!("insert_decision_state(issue_id={})", issue_id); - - db.execute( - "INSERT INTO decision_state (issue_id, initiator, start_date, end_date, current_statuses, status_history, reversibility, resolution) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) - ON CONFLICT issue_id DO NOTHING", - &[&issue_id, &initiator, &start_date, &end_date, ¤t_statuses, &status_history, &reversibility, &resolution], - ) - .await - .context("Inserting decision state")?; - - Ok(()) -} - -// pub async fn update_decision_state( -// db: &DbClient, -// issue_id: &String, -// period_end_date: &DateTime, -// current_statuses: &HashMap, -// status_history: &HashMap>, -// reversibility: &Reversibility, -// resolution: &Resolution -// ) -> Result<()> { -// tracing::trace!("update_decision_state(issue_id={})", issue_id); - -// db.execute("UPDATE decision_state SET period_end_date = $2, current_statuses = $3, status_history = $4, reversibility = $5, resolution = $6 WHERE issue_id = $1", -// &[&issue_id, &period_end_date, ¤t_statuses, &status_history, &reversibility, &resolution] -// ) -// .await -// .context("Updating decision state")?; - -// Ok(()) -// } - -// pub async fn get_decision_state_for_issue(db: &DbClient, issue_id: &String) -> Result { -// let state = db -// .query( -// "SELECT * FROM decision_state WHERE issue_id = $1", -// &[&issue_id] -// ) -// .await -// .context("Getting decision state data")?; - - -// let issue_id: String = state.get(0); -// let initiator: String = state.get(1); -// let team_members: Vec = state.get(2); -// let start_date: DateTime = state.get(3); -// let period_date: DateTime = state.get(4); -// let current_statuses: HashMap = state.get(5); -// let status_history: HashMap> = state.get(6); -// let reversibility: Reversibility = state.get(7); -// let resolution: Resolution = state.get(8); - -// Ok( -// DecisionState { -// issue_id, -// initiator, -// team_members, -// start_date, -// period_date, -// current_statuses, -// status_history, -// reversibility, -// resolution, -// } -// ) -// } diff --git a/src/db/issue_decision_state.rs b/src/db/issue_decision_state.rs new file mode 100644 index 00000000..a8120307 --- /dev/null +++ b/src/db/issue_decision_state.rs @@ -0,0 +1,112 @@ +//! The issue decision state table provides a way to store +//! the decision process state of each issue + +use serde::{Deserialize, Serialize}; +use chrono::{DateTime, FixedOffset}; +use std::collections::BTreeMap; +use parser::command::decision::{Resolution, Reversibility}; +use anyhow::{Context as _, Result}; +use tokio_postgres::Client as DbClient; + +#[derive(Debug, Serialize, Deserialize)] +pub struct IssueDecisionState { + pub issue_id: i64, + pub initiator: String, + pub start_date: DateTime, + pub end_date: DateTime, + pub current: BTreeMap, + pub history: BTreeMap>, + pub reversibility: Reversibility, + pub resolution: Resolution, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct UserStatus { + pub comment_id: String, + pub text: String, + pub reversibility: Reversibility, + pub resolution: Resolution, +} + +pub async fn insert_issue_decision_state( + db: &DbClient, + issue_number: &u64, + initiator: &String, + start_date: &DateTime, + end_date: &DateTime, + current: &BTreeMap, + history: &BTreeMap>, + reversibility: &Reversibility, + resolution: &Resolution, +) -> Result<()> { + tracing::trace!("insert_issue_decision_state(issue_id={})", issue_number); + let issue_id = *issue_number as i64; + + db.execute( + "INSERT INTO issue_decision_state (issue_id, initiator, start_date, end_date, current, history, reversibility, resolution) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + ON CONFLICT DO NOTHING", + &[&issue_id, &initiator, &start_date, &end_date, &serde_json::to_value(current).unwrap(), &serde_json::to_value(history).unwrap(), &reversibility, &resolution], + ) + .await + .context("Inserting decision state")?; + + Ok(()) +} + +pub async fn update_issue_decision_state( + db: &DbClient, + issue_number: &u64, + end_date: &DateTime, + current: &BTreeMap, + history: &BTreeMap>, + reversibility: &Reversibility, + resolution: &Resolution +) -> Result<()> { + tracing::trace!("update_issue_decision_state(issue_id={})", issue_number); + let issue_id = *issue_number as i64; + + db.execute("UPDATE issue_decision_state SET end_date = $2, current = $3, history = $4, reversibility = $5, resolution = $6 WHERE issue_id = $1", + &[&issue_id, &end_date, &serde_json::to_value(current).unwrap(), &serde_json::to_value(history).unwrap(), &reversibility, &resolution] + ) + .await + .context("Updating decision state")?; + + Ok(()) +} + +pub async fn get_issue_decision_state(db: &DbClient, issue_number: &u64) -> Result { + tracing::trace!("get_issue_decision_state(issue_id={})", issue_number); + let issue_id = *issue_number as i64; + + let state = db + .query_one( + "SELECT * FROM issue_decision_state WHERE issue_id = $1", + &[&issue_id] + ) + .await + .context("Getting decision state data")?; + + deserialize_issue_decision_state(&state) +} + +fn deserialize_issue_decision_state(row: &tokio_postgres::row::Row) -> Result { + let issue_id: i64 = row.try_get(0)?; + let initiator: String = row.try_get(1)?; + let start_date: DateTime = row.try_get(2)?; + let end_date: DateTime = row.try_get(3)?; + let current: BTreeMap = serde_json::from_value(row.try_get(4).unwrap())?; + let history: BTreeMap> = serde_json::from_value(row.try_get(5).unwrap())?; + let reversibility: Reversibility = row.try_get(6)?; + let resolution: Resolution = row.try_get(7)?; + + Ok(IssueDecisionState { + issue_id, + initiator, + start_date, + end_date, + current, + history, + reversibility, + resolution, + }) +} diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 3644c3cf..c22b0b51 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,14 +1,15 @@ use anyhow::Context as Ctx; -use parser::command::decision::{DecisionCommand, ParseError}; use crate::{ config::DecisionConfig, - github::{self, Event}, + github::{Event}, handlers::Context, interactions::ErrorComment, - db::decision_state::* + db::issue_decision_state::* }; -use std::collections::HashMap; -use chrono::{DateTime, Duration, Utc}; +use chrono::{DateTime, Duration, FixedOffset}; +use parser::command::decision::{DecisionCommand, Resolution::*, Reversibility::*}; +use std::collections::BTreeMap; +use chrono::{DateTime, FixedOffset, Duration}; // get state for issue_id from db // if no state (first call) @@ -28,15 +29,18 @@ pub(super) async fn handle_command( event: &Event, cmd: DecisionCommand, ) -> anyhow::Result<()> { + let db = ctx.db.get().await; + let DecisionCommand { resolution, reversibility } = cmd; let issue = event.issue().unwrap(); + let user = event.user(); - let is_team_member = event - .user() + let is_team_member = + user .is_team_member(&ctx.github) .await .unwrap_or(false); @@ -47,8 +51,8 @@ pub(super) async fn handle_command( return Ok(()); } - match get_decision_state(issue.id) { - Some(state) => { + match get_issue_decision_state(&db, &issue.number).await { + Ok(_state) => { // let name = match disposition { // Hold => "hold".into(), // Custom(name) => name, @@ -72,45 +76,49 @@ pub(super) async fn handle_command( // status_history, // ..state // }) - Ok(); + Ok(()) }, - None => { + _ => { match resolution { - Hold => Err(ParseError::InvalidFirstCommand), + Hold => Ok(()), // change me! Merge => { - let start_date = chrono::Utc::now().into(); - let end_date = start_date.checked_add_signed(Duration::days(10)).unwrap(); - - let current_statuses = HashMap::new(); - let status_history = HashMap::new(); + let start_date: DateTime = chrono::Utc::now().into(); + let end_date: DateTime = start_date.checked_add_signed(Duration::days(10)).unwrap(); - let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? + let mut current: BTreeMap = BTreeMap::new(); + current.insert("mcass19".to_string(), UserStatus{ + comment_id: "comment_id".to_string(), + text: "something".to_string(), + reversibility: Reversible, + resolution: Merge, + }); + let history: BTreeMap> = BTreeMap::new(); - insert_decision_state( - db, - issue.id, - user.login, - start_date, - end_date, - current_statuses, - status_history, - reversibility, - resolution, - ); + insert_issue_decision_state( + &db, + &issue.number, + &user.login, + &start_date, + &end_date, + ¤t, + &history, + &reversibility, + &Merge, + ).await?; - let comment = format!( - "Wow, it looks like you want to merge this, {}.", event.user().login - ); + // let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? let comment = format!( - "| Team member | State |\n|-------------|-------|\n| julmontesdeoca | merge |\n| mcass19 | |"); + "Wow, it looks like you want to merge this, {}.\n| Team member | State |\n|-------------|-------|\n| julmontesdeoca | merge |\n| mcass19 | |", + user.login + ); issue .post_comment(&ctx.github, &comment) .await .context("merge vote comment")?; - Ok(); + Ok(()) } } } From 50fa6427dc62aba4088c587c1148665cc81e8f10 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 9 Nov 2022 16:01:59 -0300 Subject: [PATCH 07/15] Run format --- parser/src/command.rs | 2 +- parser/src/command/decision.rs | 17 +++++----- src/db.rs | 4 +-- src/db/issue_decision_state.rs | 24 ++++++++------ src/handlers.rs | 2 +- src/handlers/decision.rs | 58 +++++++++++++++++----------------- 6 files changed, 56 insertions(+), 51 deletions(-) diff --git a/parser/src/command.rs b/parser/src/command.rs index 94fe3d54..671ea41f 100644 --- a/parser/src/command.rs +++ b/parser/src/command.rs @@ -5,6 +5,7 @@ use regex::Regex; pub mod assign; pub mod close; +pub mod decision; pub mod glacier; pub mod nominate; pub mod note; @@ -13,7 +14,6 @@ pub mod prioritize; pub mod relabel; pub mod second; pub mod shortcut; -pub mod decision; #[derive(Debug, PartialEq)] pub enum Command<'a> { diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 1d24829d..9e55e27b 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -8,26 +8,27 @@ //! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. //! ``` -use crate::token::{Token, Tokenizer}; use crate::error::Error; -use serde::{Deserialize, Serialize}; +use crate::token::{Token, Tokenizer}; use postgres_types::{FromSql, ToSql}; +use serde::{Deserialize, Serialize}; /// A command as parsed and received from calling the bot with some arguments, /// like `@rustbot merge` #[derive(Debug, Eq, PartialEq)] pub struct DecisionCommand { pub resolution: Resolution, - pub reversibility: Reversibility + pub reversibility: Reversibility, } impl DecisionCommand { pub fn parse<'a>(input: &mut Tokenizer<'a>) -> Result, Error<'a>> { if let Some(Token::Word("merge")) = input.peek_token()? { - Ok(Some(Self { - resolution: Resolution::Merge, - reversibility: Reversibility::Reversible - })) } else { + Ok(Some(Self { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible, + })) + } else { Ok(None) } } @@ -35,7 +36,7 @@ impl DecisionCommand { #[derive(Debug, Eq, PartialEq)] pub enum ParseError { - InvalidFirstCommand + InvalidFirstCommand, } #[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] diff --git a/src/db.rs b/src/db.rs index 8e82f866..1a83a8bf 100644 --- a/src/db.rs +++ b/src/db.rs @@ -8,8 +8,8 @@ use std::sync::{Arc, Mutex}; use tokio::sync::{OwnedSemaphorePermit, Semaphore}; use tokio_postgres::Client as DbClient; -pub mod issue_decision_state; pub mod issue_data; +pub mod issue_decision_state; pub mod jobs; pub mod notifications; pub mod rustc_commits; @@ -280,7 +280,7 @@ CREATE TYPE reversibility AS ENUM ('reversible', 'irreversible'); " CREATE TYPE resolution AS ENUM ('hold', 'merge'); ", -"CREATE TABLE issue_decision_state ( + "CREATE TABLE issue_decision_state ( issue_id BIGINT PRIMARY KEY, initiator TEXT NOT NULL, start_date TIMESTAMP WITH TIME ZONE NOT NULL, diff --git a/src/db/issue_decision_state.rs b/src/db/issue_decision_state.rs index a8120307..62973948 100644 --- a/src/db/issue_decision_state.rs +++ b/src/db/issue_decision_state.rs @@ -1,11 +1,11 @@ -//! The issue decision state table provides a way to store +//! The issue decision state table provides a way to store //! the decision process state of each issue -use serde::{Deserialize, Serialize}; +use anyhow::{Context as _, Result}; use chrono::{DateTime, FixedOffset}; -use std::collections::BTreeMap; use parser::command::decision::{Resolution, Reversibility}; -use anyhow::{Context as _, Result}; +use serde::{Deserialize, Serialize}; +use std::collections::BTreeMap; use tokio_postgres::Client as DbClient; #[derive(Debug, Serialize, Deserialize)] @@ -54,13 +54,13 @@ pub async fn insert_issue_decision_state( } pub async fn update_issue_decision_state( - db: &DbClient, + db: &DbClient, issue_number: &u64, end_date: &DateTime, current: &BTreeMap, history: &BTreeMap>, reversibility: &Reversibility, - resolution: &Resolution + resolution: &Resolution, ) -> Result<()> { tracing::trace!("update_issue_decision_state(issue_id={})", issue_number); let issue_id = *issue_number as i64; @@ -74,18 +74,21 @@ pub async fn update_issue_decision_state( Ok(()) } -pub async fn get_issue_decision_state(db: &DbClient, issue_number: &u64) -> Result { +pub async fn get_issue_decision_state( + db: &DbClient, + issue_number: &u64, +) -> Result { tracing::trace!("get_issue_decision_state(issue_id={})", issue_number); let issue_id = *issue_number as i64; let state = db .query_one( "SELECT * FROM issue_decision_state WHERE issue_id = $1", - &[&issue_id] + &[&issue_id], ) .await .context("Getting decision state data")?; - + deserialize_issue_decision_state(&state) } @@ -95,7 +98,8 @@ fn deserialize_issue_decision_state(row: &tokio_postgres::row::Row) -> Result = row.try_get(2)?; let end_date: DateTime = row.try_get(3)?; let current: BTreeMap = serde_json::from_value(row.try_get(4).unwrap())?; - let history: BTreeMap> = serde_json::from_value(row.try_get(5).unwrap())?; + let history: BTreeMap> = + serde_json::from_value(row.try_get(5).unwrap())?; let reversibility: Reversibility = row.try_get(6)?; let resolution: Resolution = row.try_get(7)?; diff --git a/src/handlers.rs b/src/handlers.rs index 1257dcc5..b948486e 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -26,6 +26,7 @@ impl fmt::Display for HandlerError { mod assign; mod autolabel; mod close; +mod decision; mod github_releases; mod glacier; pub mod jobs; @@ -44,7 +45,6 @@ mod review_submitted; mod rfc_helper; mod rustc_commits; mod shortcut; -mod decision; pub async fn handle(ctx: &Context, event: &Event) -> Vec { let config = config::get(&ctx.github, event.repo()).await; diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index c22b0b51..2841ede5 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,15 +1,11 @@ -use anyhow::Context as Ctx; use crate::{ - config::DecisionConfig, - github::{Event}, - handlers::Context, + config::DecisionConfig, db::issue_decision_state::*, github::Event, handlers::Context, interactions::ErrorComment, - db::issue_decision_state::* }; +use anyhow::Context as Ctx; use chrono::{DateTime, Duration, FixedOffset}; use parser::command::decision::{DecisionCommand, Resolution::*, Reversibility::*}; use std::collections::BTreeMap; -use chrono::{DateTime, FixedOffset, Duration}; // get state for issue_id from db // if no state (first call) @@ -33,20 +29,19 @@ pub(super) async fn handle_command( let DecisionCommand { resolution, - reversibility + reversibility, } = cmd; let issue = event.issue().unwrap(); let user = event.user(); - let is_team_member = - user - .is_team_member(&ctx.github) - .await - .unwrap_or(false); + let is_team_member = user.is_team_member(&ctx.github).await.unwrap_or(false); if !is_team_member { - let cmnt = ErrorComment::new(&issue, "Only team members can be part of the decision process."); + let cmnt = ErrorComment::new( + &issue, + "Only team members can be part of the decision process.", + ); cmnt.post(&ctx.github).await?; return Ok(()); } @@ -57,41 +52,45 @@ pub(super) async fn handle_command( // Hold => "hold".into(), // Custom(name) => name, // }; - + // let mut current_statuses = state.current_statuses; // let mut status_history = state.status_history; - + // if let Some(entry) = current_statuses.get_mut(&user) { // let past = status_history.entry(user).or_insert(Vec::new()); - + // past.push(entry.clone()); - + // *entry = UserStatus::new(name, issue_id, comment_id); // } else { // current_statuses.insert(user, UserStatus::new("hold".into(), issue_id, comment_id)); // } - + // Ok(State { // current_statuses, // status_history, // ..state // }) Ok(()) - }, + } _ => { match resolution { Hold => Ok(()), // change me! Merge => { let start_date: DateTime = chrono::Utc::now().into(); - let end_date: DateTime = start_date.checked_add_signed(Duration::days(10)).unwrap(); + let end_date: DateTime = + start_date.checked_add_signed(Duration::days(10)).unwrap(); let mut current: BTreeMap = BTreeMap::new(); - current.insert("mcass19".to_string(), UserStatus{ - comment_id: "comment_id".to_string(), - text: "something".to_string(), - reversibility: Reversible, - resolution: Merge, - }); + current.insert( + "mcass19".to_string(), + UserStatus { + comment_id: "comment_id".to_string(), + text: "something".to_string(), + reversibility: Reversible, + resolution: Merge, + }, + ); let history: BTreeMap> = BTreeMap::new(); insert_issue_decision_state( @@ -104,15 +103,16 @@ pub(super) async fn handle_command( &history, &reversibility, &Merge, - ).await?; + ) + .await?; // let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? - + let comment = format!( "Wow, it looks like you want to merge this, {}.\n| Team member | State |\n|-------------|-------|\n| julmontesdeoca | merge |\n| mcass19 | |", user.login ); - + issue .post_comment(&ctx.github, &comment) .await From 0baf9e64ca77121ff1ae6b487ea76cd13389924c Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Tue, 6 Dec 2022 11:05:51 -0300 Subject: [PATCH 08/15] Schedule job for initial merge --- parser/src/command/decision.rs | 2 +- src/db/issue_decision_state.rs | 16 +++++----- src/github.rs | 25 +++++++++++++-- src/handlers/decision.rs | 57 +++++++++++++++++++++++++--------- src/handlers/jobs.rs | 34 ++++++++++++++++++++ 5 files changed, 108 insertions(+), 26 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 9e55e27b..fe237a2a 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -48,7 +48,7 @@ pub enum Reversibility { Irreversible, } -#[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, Copy, ToSql, FromSql, Eq, PartialEq)] #[postgres(name = "resolution")] pub enum Resolution { #[postgres(name = "merge")] diff --git a/src/db/issue_decision_state.rs b/src/db/issue_decision_state.rs index 62973948..5091880b 100644 --- a/src/db/issue_decision_state.rs +++ b/src/db/issue_decision_state.rs @@ -2,7 +2,7 @@ //! the decision process state of each issue use anyhow::{Context as _, Result}; -use chrono::{DateTime, FixedOffset}; +use chrono::{DateTime, Utc}; use parser::command::decision::{Resolution, Reversibility}; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; @@ -12,8 +12,8 @@ use tokio_postgres::Client as DbClient; pub struct IssueDecisionState { pub issue_id: i64, pub initiator: String, - pub start_date: DateTime, - pub end_date: DateTime, + pub start_date: DateTime, + pub end_date: DateTime, pub current: BTreeMap, pub history: BTreeMap>, pub reversibility: Reversibility, @@ -32,8 +32,8 @@ pub async fn insert_issue_decision_state( db: &DbClient, issue_number: &u64, initiator: &String, - start_date: &DateTime, - end_date: &DateTime, + start_date: &DateTime, + end_date: &DateTime, current: &BTreeMap, history: &BTreeMap>, reversibility: &Reversibility, @@ -56,7 +56,7 @@ pub async fn insert_issue_decision_state( pub async fn update_issue_decision_state( db: &DbClient, issue_number: &u64, - end_date: &DateTime, + end_date: &DateTime, current: &BTreeMap, history: &BTreeMap>, reversibility: &Reversibility, @@ -95,8 +95,8 @@ pub async fn get_issue_decision_state( fn deserialize_issue_decision_state(row: &tokio_postgres::row::Row) -> Result { let issue_id: i64 = row.try_get(0)?; let initiator: String = row.try_get(1)?; - let start_date: DateTime = row.try_get(2)?; - let end_date: DateTime = row.try_get(3)?; + let start_date: DateTime = row.try_get(2)?; + let end_date: DateTime = row.try_get(3)?; let current: BTreeMap = serde_json::from_value(row.try_get(4).unwrap())?; let history: BTreeMap> = serde_json::from_value(row.try_get(5).unwrap())?; diff --git a/src/github.rs b/src/github.rs index 295db372..7c1820c8 100644 --- a/src/github.rs +++ b/src/github.rs @@ -402,7 +402,7 @@ impl fmt::Display for IssueRepository { } impl IssueRepository { - fn url(&self) -> String { + pub fn url(&self) -> String { format!( "https://api.github.com/repos/{}/{}", self.organization, self.repository @@ -779,6 +779,27 @@ impl Issue { Ok(()) } + pub async fn merge(&self, client: &GithubClient) -> anyhow::Result<()> { + let merge_url = format!("{}/pulls/{}/merge", self.repository().url(), self.number); + + // change defaults by reading from somewhere, maybe in .toml? + #[derive(serde::Serialize)] + struct MergeIssue<'a> { + commit_title: &'a str, + merge_method: &'a str + } + + client + ._send_req(client.put(&merge_url).json(&MergeIssue { + commit_title: "Merged by the bot!", + merge_method: "merge" + })) + .await + .context("failed to merge issue")?; + + Ok(()) + } + /// Returns the diff in this event, for Open and Synchronize events for now. pub async fn diff(&self, client: &GithubClient) -> anyhow::Result> { let (before, after) = if let (Some(base), Some(head)) = (&self.base, &self.head) { @@ -1501,7 +1522,7 @@ impl GithubClient { response.text().await.context("raw gist from url") } - fn get(&self, url: &str) -> RequestBuilder { + pub fn get(&self, url: &str) -> RequestBuilder { log::trace!("get {:?}", url); self.client.get(url).configure(self) } diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 2841ede5..6cf6c9e5 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,23 +1,35 @@ +use crate::db::jobs::*; use crate::{ config::DecisionConfig, db::issue_decision_state::*, github::Event, handlers::Context, interactions::ErrorComment, }; use anyhow::Context as Ctx; -use chrono::{DateTime, Duration, FixedOffset}; -use parser::command::decision::{DecisionCommand, Resolution::*, Reversibility::*}; +use chrono::{DateTime, Duration, Utc}; +use parser::command::decision::Resolution::{Hold, Merge}; +use parser::command::decision::*; +use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; // get state for issue_id from db -// if no state (first call) - // initialize state - // schedule job if necessary - // send comment to github - // save state -// if state - // apply logic to decide what to do - // schedule job if necessary - // send comment to github - // save state + // if no state (first call) + // initialize state + // schedule job if necessary + // send comment to github + // save state + // if state + // apply logic to decide what to do + // schedule job if necessary + // send comment to github + // save state + +pub const DECISION_PROCESS_JOB_NAME: &str = "decision_process_action"; + +#[derive(Serialize, Deserialize)] +pub struct DecisionProcessActionMetadata { + pub message: String, + pub get_issue_url: String, + pub status: Resolution, +} pub(super) async fn handle_command( ctx: &Context, @@ -77,8 +89,8 @@ pub(super) async fn handle_command( match resolution { Hold => Ok(()), // change me! Merge => { - let start_date: DateTime = chrono::Utc::now().into(); - let end_date: DateTime = + let start_date: DateTime = chrono::Utc::now().into(); + let end_date: DateTime = start_date.checked_add_signed(Duration::days(10)).unwrap(); let mut current: BTreeMap = BTreeMap::new(); @@ -87,7 +99,7 @@ pub(super) async fn handle_command( UserStatus { comment_id: "comment_id".to_string(), text: "something".to_string(), - reversibility: Reversible, + reversibility: Reversibility::Reversible, resolution: Merge, }, ); @@ -106,6 +118,21 @@ pub(super) async fn handle_command( ) .await?; + let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { + message: "some message".to_string(), + get_issue_url: format!("{}/issues/{}", issue.repository().url(), issue.number), + status: Merge, + }) + .unwrap(); + + insert_job( + &db, + &DECISION_PROCESS_JOB_NAME.to_string(), + &end_date, + &metadata, + ) + .await?; + // let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? let comment = format!( diff --git a/src/handlers/jobs.rs b/src/handlers/jobs.rs index d37f0bde..1b111d21 100644 --- a/src/handlers/jobs.rs +++ b/src/handlers/jobs.rs @@ -4,8 +4,17 @@ // Further info could be find in src/jobs.rs +use crate::github::*; +use crate::handlers::decision::{DecisionProcessActionMetadata, DECISION_PROCESS_JOB_NAME}; +use parser::command::decision::Resolution::Merge; +use reqwest::Client; +use tracing as log; + pub async fn handle_job(name: &String, metadata: &serde_json::Value) -> anyhow::Result<()> { match name { + matched_name if *matched_name == DECISION_PROCESS_JOB_NAME.to_string() => { + decision_process_handler(&metadata).await + } _ => default(&name, &metadata), } } @@ -19,3 +28,28 @@ fn default(name: &String, metadata: &serde_json::Value) -> anyhow::Result<()> { Ok(()) } + +async fn decision_process_handler(metadata: &serde_json::Value) -> anyhow::Result<()> { + tracing::trace!( + "handle_job fell into decision process case: (metadata={:?})", + metadata + ); + + let metadata: DecisionProcessActionMetadata = serde_json::from_value(metadata.clone())?; + + match metadata.status { + Merge => { + let gh_client = GithubClient::new_with_default_token(Client::new().clone()); + + let request = gh_client.get(&metadata.get_issue_url); + + match gh_client.json::(request).await { + Ok(issue) => issue.merge(&gh_client).await?, + Err(e) => log::error!("Failed to get issue {}, error: {}", metadata.get_issue_url, e), + } + } + _ => {} + } + + Ok(()) +} From 393db241e6db1277e138c896e5b99f0b2eea0ae0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Montes=20de=20Oca?= Date: Thu, 8 Dec 2022 13:29:19 -0300 Subject: [PATCH 09/15] Merge pull request from julmontesdeoca/parser-for-merge-and-hold * add initial parsing for merge and hold commands * cleanup format checking in parser --- parser/src/command/decision.rs | 104 ++++++++++++++++++++++++++++++--- 1 file changed, 95 insertions(+), 9 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index fe237a2a..5857a500 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -8,6 +8,8 @@ //! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. //! ``` +use std::fmt; + use crate::error::Error; use crate::token::{Token, Tokenizer}; use postgres_types::{FromSql, ToSql}; @@ -23,20 +25,49 @@ pub struct DecisionCommand { impl DecisionCommand { pub fn parse<'a>(input: &mut Tokenizer<'a>) -> Result, Error<'a>> { - if let Some(Token::Word("merge")) = input.peek_token()? { - Ok(Some(Self { - resolution: Resolution::Merge, - reversibility: Reversibility::Reversible, - })) - } else { - Ok(None) + let mut toks = input.clone(); + + match toks.peek_token()? { + Some(Token::Word("merge")) => { + command_or_error(input, &mut toks, Self { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible, + }) + } + Some(Token::Word("hold")) => { + command_or_error(input, &mut toks, Self { + resolution: Resolution::Hold, + reversibility: Reversibility::Reversible, + }) + } + _ => Ok(None), } } } -#[derive(Debug, Eq, PartialEq)] +fn command_or_error<'a>(input: &mut Tokenizer<'a>, toks: &mut Tokenizer<'a>, command: DecisionCommand) -> Result, Error<'a>> { + toks.next_token()?; + if let Some(Token::Dot) | Some(Token::EndOfLine) = toks.peek_token()? { + *input = toks.clone(); + Ok(Some(command)) + } else { + Err(toks.error(ParseError::ExpectedEnd)) + } +} + +#[derive(PartialEq, Eq, Debug)] pub enum ParseError { - InvalidFirstCommand, + ExpectedEnd, +} + +impl std::error::Error for ParseError {} + +impl fmt::Display for ParseError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + ParseError::ExpectedEnd => write!(f, "expected end of command"), + } + } } #[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] @@ -56,3 +87,58 @@ pub enum Resolution { #[postgres(name = "hold")] Hold, } +#[cfg(test)] +mod tests { + use super::*; + + fn parse<'a>(input: &'a str) -> Result, Error<'a>> { + let mut toks = Tokenizer::new(input); + Ok(DecisionCommand::parse(&mut toks)?) + } + + #[test] + fn test_correct_merge() { + assert_eq!( + parse("merge"), + Ok(Some(DecisionCommand { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible + })), + ); + } + + #[test] + fn test_correct_merge_final_dot() { + assert_eq!( + parse("merge."), + Ok(Some(DecisionCommand { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible + })), + ); + } + + #[test] + fn test_correct_hold() { + assert_eq!( + parse("hold"), + Ok(Some(DecisionCommand { + resolution: Resolution::Hold, + reversibility: Reversibility::Reversible + })), + ); + } + + #[test] + fn test_expected_end() { + use std::error::Error; + assert_eq!( + parse("hold my beer") + .unwrap_err() + .source() + .unwrap() + .downcast_ref(), + Some(&ParseError::ExpectedEnd), + ); + } +} From 6fd8baab6312b0e83beb489843c2941cf2832fc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Montes=20de=20Oca?= Date: Fri, 9 Dec 2022 10:38:20 -0300 Subject: [PATCH 10/15] Merge master * Automate documentation updates for rust-lang/rust * docs-update: Switch to using a separate repo for creating the commits. * Fix recent_commits pagination. * Automatically re-scan GitHub commits every 30 minutes This lets us recover from faults in the GitHub API more quickly. * Do not expect sha field in API response We should eventually add in-CI testing for this kind of oops (it's fairly common when we change these) or move to auto-generated GitHub API code -- but for now this fixes commit fetching. Co-authored-by: Eric Huss Co-authored-by: Mark Rousskov --- github-graphql/src/lib.rs | 151 +++++++++++- src/db.rs | 8 +- src/github.rs | 430 +++++++++++++++++++++++++++++++++- src/handlers.rs | 3 +- src/handlers/docs_update.rs | 201 ++++++++++++++++ src/handlers/jobs.rs | 16 +- src/handlers/rustc_commits.rs | 34 ++- src/jobs.rs | 10 +- src/main.rs | 30 +-- 9 files changed, 852 insertions(+), 31 deletions(-) create mode 100644 src/handlers/docs_update.rs diff --git a/github-graphql/src/lib.rs b/github-graphql/src/lib.rs index b508dcb0..6ccf10e7 100644 --- a/github-graphql/src/lib.rs +++ b/github-graphql/src/lib.rs @@ -2,7 +2,7 @@ //! //! See for more GitHub's GraphQL API. -// This schema can be downloaded from https://docs.github.com/en/graphql/overview/public-schema +// This schema can be downloaded from https://docs.github.com/public/schema.docs.graphql #[cynic::schema_for_derives(file = "src/github.graphql", module = "schema")] pub mod queries { use super::schema; @@ -155,6 +155,155 @@ pub mod queries { pub struct Uri(pub String); } +#[cynic::schema_for_derives(file = "src/github.graphql", module = "schema")] +pub mod docs_update_queries { + use super::queries::{DateTime, PageInfo}; + use super::schema; + + #[derive(cynic::FragmentArguments, Debug)] + pub struct RecentCommitsArguments { + pub branch: String, + pub name: String, + pub owner: String, + pub after: Option, + } + + /// Query for fetching recent commits and their associated PRs. + /// + /// This query is built from: + /// + /// ```text + /// query RecentCommits($name: String!, $owner: String!, $branch: String!, $after: String) { + /// repository(name: $name, owner: $owner) { + /// ref(qualifiedName: $branch) { + /// target { + /// ... on Commit { + /// history(first: 100, after: $after) { + /// totalCount + /// pageInfo { + /// hasNextPage + /// endCursor + /// } + /// nodes { + /// oid + /// parents(first: 1) { + /// nodes { + /// oid + /// } + /// } + /// committedDate + /// messageHeadline + /// associatedPullRequests(first: 1) { + /// nodes { + /// number + /// title + /// } + /// } + /// } + /// } + /// } + /// } + /// } + /// } + /// } + /// ``` + #[derive(cynic::QueryFragment, Debug)] + #[cynic(graphql_type = "Query", argument_struct = "RecentCommitsArguments")] + pub struct RecentCommits { + #[arguments(name = &args.name, owner = &args.owner)] + pub repository: Option, + } + + #[derive(cynic::QueryFragment, Debug)] + #[cynic(argument_struct = "RecentCommitsArguments")] + pub struct Repository { + #[arguments(qualified_name = &args.branch)] + #[cynic(rename = "ref")] + pub ref_: Option, + } + + #[derive(cynic::QueryFragment, Debug)] + #[cynic(argument_struct = "RecentCommitsArguments")] + pub struct Ref { + pub target: Option, + } + + #[derive(cynic::QueryFragment, Debug)] + #[cynic(argument_struct = "RecentCommitsArguments")] + pub struct Commit { + #[arguments(first = 100, after = &args.after)] + pub history: CommitHistoryConnection, + } + + #[derive(cynic::QueryFragment, Debug)] + pub struct CommitHistoryConnection { + pub total_count: i32, + pub page_info: PageInfo, + pub nodes: Option>>, + } + + #[derive(cynic::QueryFragment, Debug)] + #[cynic(graphql_type = "Commit")] + pub struct Commit2 { + pub oid: GitObjectID, + #[arguments(first = 1)] + pub parents: CommitConnection, + pub committed_date: DateTime, + pub message_headline: String, + #[arguments(first = 1)] + pub associated_pull_requests: Option, + } + + #[derive(cynic::QueryFragment, Debug)] + pub struct PullRequestConnection { + pub nodes: Option>>, + } + + #[derive(cynic::QueryFragment, Debug)] + pub struct PullRequest { + pub number: i32, + pub title: String, + } + + #[derive(cynic::QueryFragment, Debug)] + pub struct CommitConnection { + pub nodes: Option>>, + } + + #[derive(cynic::QueryFragment, Debug)] + #[cynic(graphql_type = "Commit")] + pub struct Commit3 { + pub oid: GitObjectID, + } + + #[derive(cynic::InlineFragments, Debug)] + #[cynic(argument_struct = "RecentCommitsArguments")] + pub enum GitObject { + Commit(Commit), + // These three variants are here just to pacify cynic. I don't know + // why it fails to compile without them. + Tree(Tree), + Tag(Tag), + Blob(Blob), + } + + #[derive(cynic::QueryFragment, Debug)] + pub struct Tree { + pub id: cynic::Id, + } + #[derive(cynic::QueryFragment, Debug)] + pub struct Tag { + pub id: cynic::Id, + } + #[derive(cynic::QueryFragment, Debug)] + pub struct Blob { + pub id: cynic::Id, + } + + #[derive(cynic::Scalar, Debug, Clone)] + pub struct GitObjectID(pub String); +} + mod schema { cynic::use_schema!("src/github.graphql"); } diff --git a/src/db.rs b/src/db.rs index 1a83a8bf..bd3de5cc 100644 --- a/src/db.rs +++ b/src/db.rs @@ -1,5 +1,5 @@ -use crate::db::jobs::*; use crate::handlers::jobs::handle_job; +use crate::{db::jobs::*, handlers::Context}; use anyhow::Context as _; use chrono::Utc; use native_tls::{Certificate, TlsConnector}; @@ -199,20 +199,20 @@ pub async fn schedule_jobs(db: &DbClient, jobs: Vec) -> anyhow::Res Ok(()) } -pub async fn run_scheduled_jobs(db: &DbClient) -> anyhow::Result<()> { +pub async fn run_scheduled_jobs(ctx: &Context, db: &DbClient) -> anyhow::Result<()> { let jobs = get_jobs_to_execute(&db).await.unwrap(); tracing::trace!("jobs to execute: {:#?}", jobs); for job in jobs.iter() { update_job_executed_at(&db, &job.id).await?; - match handle_job(&job.name, &job.metadata).await { + match handle_job(&ctx, &job.name, &job.metadata).await { Ok(_) => { tracing::trace!("job successfully executed (id={})", job.id); delete_job(&db, &job.id).await?; } Err(e) => { - tracing::trace!("job failed on execution (id={:?}, error={:?})", job.id, e); + tracing::error!("job failed on execution (id={:?}, error={:?})", job.id, e); update_job_error_message(&db, &job.id, &e.to_string()).await?; } } diff --git a/src/github.rs b/src/github.rs index 7c1820c8..40cc1305 100644 --- a/src/github.rs +++ b/src/github.rs @@ -6,7 +6,7 @@ use hyper::header::HeaderValue; use once_cell::sync::OnceCell; use reqwest::header::{AUTHORIZATION, USER_AGENT}; use reqwest::{Client, Request, RequestBuilder, Response, StatusCode}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::convert::TryInto; use std::{ fmt, @@ -1028,6 +1028,10 @@ impl Repository { const GITHUB_API_URL: &'static str = "https://api.github.com"; const GITHUB_GRAPHQL_API_URL: &'static str = "https://api.github.com/graphql"; + fn url(&self) -> String { + format!("{}/repos/{}", Repository::GITHUB_API_URL, self.full_name) + } + pub fn owner(&self) -> &str { self.full_name.split_once('/').unwrap().0 } @@ -1201,6 +1205,338 @@ impl Repository { ordering.page, ) } + + /// Retrieves a git commit for the given SHA. + pub async fn git_commit(&self, client: &GithubClient, sha: &str) -> anyhow::Result { + let url = format!("{}/git/commits/{sha}", self.url()); + client + .json(client.get(&url)) + .await + .with_context(|| format!("{} failed to get git commit {sha}", self.full_name)) + } + + /// Creates a new commit. + pub async fn create_commit( + &self, + client: &GithubClient, + message: &str, + parents: &[&str], + tree: &str, + ) -> anyhow::Result { + let url = format!("{}/git/commits", self.url()); + client + .json(client.post(&url).json(&serde_json::json!({ + "message": message, + "parents": parents, + "tree": tree, + }))) + .await + .with_context(|| format!("{} failed to create commit for tree {tree}", self.full_name)) + } + + /// Retrieves a git reference for the given refname. + pub async fn get_reference( + &self, + client: &GithubClient, + refname: &str, + ) -> anyhow::Result { + let url = format!("{}/git/ref/{}", self.url(), refname); + client + .json(client.get(&url)) + .await + .with_context(|| format!("{} failed to get git reference {refname}", self.full_name)) + } + + /// Updates an existing git reference to a new SHA. + pub async fn update_reference( + &self, + client: &GithubClient, + refname: &str, + sha: &str, + ) -> anyhow::Result<()> { + let url = format!("{}/git/refs/{}", self.url(), refname); + client + ._send_req(client.patch(&url).json(&serde_json::json!({ + "sha": sha, + "force": true, + }))) + .await + .with_context(|| { + format!( + "{} failed to update reference {refname} to {sha}", + self.full_name + ) + })?; + Ok(()) + } + + /// Returns a list of recent commits on the given branch. + /// + /// Returns results in the OID range `oldest` (exclusive) to `newest` + /// (inclusive). + pub async fn recent_commits( + &self, + client: &GithubClient, + branch: &str, + oldest: &str, + newest: &str, + ) -> anyhow::Result> { + // This is used to deduplicate the results (so that a PR with multiple + // commits will only show up once). + let mut prs_seen = HashSet::new(); + let mut recent_commits = Vec::new(); // This is the final result. + use cynic::QueryBuilder; + use github_graphql::docs_update_queries::{ + GitObject, RecentCommits, RecentCommitsArguments, + }; + + let mut args = RecentCommitsArguments { + branch: branch.to_string(), + name: self.name().to_string(), + owner: self.owner().to_string(), + after: None, + }; + let mut found_newest = false; + let mut found_oldest = false; + // This simulates --first-parent. We only care about top-level commits. + // Unfortunately the GitHub API doesn't provide anything like that. + let mut next_first_parent = None; + // Search for `oldest` within 3 pages (300 commits). + for _ in 0..3 { + let query = RecentCommits::build(&args); + let response = client + .json(client.post(Repository::GITHUB_GRAPHQL_API_URL).json(&query)) + .await + .with_context(|| { + format!( + "{} failed to get recent commits branch={branch}", + self.full_name + ) + })?; + let data: cynic::GraphQlResponse = query + .decode_response(response) + .with_context(|| format!("failed to parse response for `RecentCommits`"))?; + if let Some(errors) = data.errors { + anyhow::bail!("There were graphql errors. {:?}", errors); + } + let target = data + .data + .ok_or_else(|| anyhow::anyhow!("No data returned."))? + .repository + .ok_or_else(|| anyhow::anyhow!("No repository."))? + .ref_ + .ok_or_else(|| anyhow::anyhow!("No ref."))? + .target + .ok_or_else(|| anyhow::anyhow!("No target."))?; + let commit = match target { + GitObject::Commit(commit) => commit, + _ => anyhow::bail!("unexpected target type {target:?}"), + }; + let commits = commit + .history + .nodes + .ok_or_else(|| anyhow::anyhow!("No history."))? + .into_iter() + .filter_map(|node| node) + // Don't include anything newer than `newest` + .skip_while(|node| { + if found_newest || node.oid.0 == newest { + found_newest = true; + false + } else { + // This should only happen if there is a commit that arrives + // between the time that `update_submodules` fetches the latest + // ref, and this runs. This window should be a few seconds, so it + // should be unlikely. This warning is here in case my assumptions + // about how things work is not correct. + tracing::warn!( + "unexpected race with submodule history, newest oid={newest} skipping oid={}", + node.oid.0 + ); + true + } + }) + // Skip nodes that aren't the first parent + .filter(|node| { + let this_first_parent = node.parents.nodes + .as_ref() + // Grab the first parent + .and_then(|nodes| nodes.first()) + // Strip away the useless Option + .and_then(|parent_opt| parent_opt.as_ref()) + .map(|parent| parent.oid.0.clone()); + + match &next_first_parent { + Some(first_parent) => { + if first_parent == &node.oid.0 { + // Found the next first parent, include it and + // set next_first_parent to look for this + // commit's first parent. + next_first_parent = this_first_parent; + true + } else { + // Still looking for the next first parent. + false + } + } + None => { + // First commit. + next_first_parent = this_first_parent; + true + } + } + }) + // Stop once reached the `oldest` commit + .take_while(|node| { + if node.oid.0 == oldest { + found_oldest = true; + false + } else { + true + } + }) + .filter_map(|node| { + // Determine if this is associated with a PR or not. + match node.associated_pull_requests + // Strip away the useless Option + .and_then(|pr| pr.nodes) + // Get the first PR (we only care about one) + .and_then(|mut nodes| nodes.pop()) + // Strip away the useless Option + .flatten() { + Some(pr) => { + // Only include a PR once + if prs_seen.insert(pr.number) { + Some(RecentCommit { + pr_num: Some(pr.number), + title: pr.title, + oid: node.oid.0.clone(), + committed_date: node.committed_date, + }) + } else { + None + } + } + None => { + // This is an unassociated commit, possibly + // created without a PR. + Some(RecentCommit { + pr_num: None, + title: node.message_headline, + oid: node.oid.0, + committed_date: node.committed_date, + }) + } + } + }); + recent_commits.extend(commits); + let page_info = commit.history.page_info; + if found_oldest || !page_info.has_next_page || page_info.end_cursor.is_none() { + break; + } + args.after = page_info.end_cursor; + } + if !found_oldest { + // This should probably do something more than log a warning, but + // I don't think it is too important at this time (the log message + // is only informational, and this should be unlikely to happen). + tracing::warn!( + "{} failed to find oldest commit sha={oldest} branch={branch}", + self.full_name + ); + } + Ok(recent_commits) + } + + /// Creates a new git tree based on another tree. + pub async fn update_tree( + &self, + client: &GithubClient, + base_tree: &str, + tree: &[GitTreeEntry], + ) -> anyhow::Result { + let url = format!("{}/git/trees", self.url()); + client + .json(client.post(&url).json(&serde_json::json!({ + "base_tree": base_tree, + "tree": tree, + }))) + .await + .with_context(|| { + format!( + "{} failed to update tree with base {base_tree}", + self.full_name + ) + }) + } + + /// Returns information about the git submodule at the given path. + /// + /// `refname` is the ref to use for fetching information. If `None`, will + /// use the latest version on the default branch. + pub async fn submodule( + &self, + client: &GithubClient, + path: &str, + refname: Option<&str>, + ) -> anyhow::Result { + let mut url = format!("{}/contents/{}", self.url(), path); + if let Some(refname) = refname { + url.push_str("?ref="); + url.push_str(refname); + } + client.json(client.get(&url)).await.with_context(|| { + format!( + "{} failed to get submodule path={path} refname={refname:?}", + self.full_name + ) + }) + } + + /// Creates a new PR. + pub async fn new_pr( + &self, + client: &GithubClient, + title: &str, + head: &str, + base: &str, + body: &str, + ) -> anyhow::Result { + let url = format!("{}/pulls", self.url()); + let mut issue: Issue = client + .json(client.post(&url).json(&serde_json::json!({ + "title": title, + "head": head, + "base": base, + "body": body, + }))) + .await + .with_context(|| { + format!( + "{} failed to create a new PR head={head} base={base} title={title}", + self.full_name + ) + })?; + issue.pull_request = Some(PullRequestDetails {}); + Ok(issue) + } + + /// Synchronize a branch (in a forked repository) by pulling in its upstream contents. + pub async fn merge_upstream(&self, client: &GithubClient, branch: &str) -> anyhow::Result<()> { + let url = format!("{}/merge-upstream", self.url()); + client + ._send_req(client.post(&url).json(&serde_json::json!({ + "branch": branch, + }))) + .await + .with_context(|| { + format!( + "{} failed to merge upstream branch {branch}", + self.full_name + ) + })?; + Ok(()) + } } pub struct Query<'a> { @@ -1598,19 +1934,64 @@ impl GithubClient { } } } + + /// Returns information about a repository. + /// + /// The `full_name` should be something like `rust-lang/rust`. + pub async fn repository(&self, full_name: &str) -> anyhow::Result { + let req = self.get(&format!("{}/repos/{full_name}", Repository::GITHUB_API_URL)); + self.json(req) + .await + .with_context(|| format!("{} failed to get repo", full_name)) + } } #[derive(Debug, serde::Deserialize)] pub struct GithubCommit { pub sha: String, - pub commit: GitCommit, + pub commit: GithubCommitCommitField, pub parents: Vec, } +#[derive(Debug, serde::Deserialize)] +pub struct GithubCommitCommitField { + pub author: GitUser, + pub message: String, + pub tree: GitCommitTree, +} + #[derive(Debug, serde::Deserialize)] pub struct GitCommit { + pub sha: String, pub author: GitUser, pub message: String, + pub tree: GitCommitTree, +} + +#[derive(Debug, serde::Deserialize)] +pub struct GitCommitTree { + pub sha: String, +} + +#[derive(Debug, serde::Deserialize)] +pub struct GitTreeObject { + pub sha: String, +} + +#[derive(Debug, serde::Serialize, serde::Deserialize)] +pub struct GitTreeEntry { + pub path: String, + pub mode: String, + #[serde(rename = "type")] + pub object_type: String, + pub sha: String, +} + +pub struct RecentCommit { + pub title: String, + pub pr_num: Option, + pub oid: String, + pub committed_date: DateTime, } #[derive(Debug, serde::Deserialize)] @@ -1804,6 +2185,51 @@ impl IssuesQuery for LeastRecentlyReviewedPullRequests { } } +#[derive(Debug, serde::Deserialize)] +pub struct GitReference { + #[serde(rename = "ref")] + pub refname: String, + pub object: GitObject, +} + +#[derive(Debug, serde::Deserialize)] +pub struct GitObject { + #[serde(rename = "type")] + pub object_type: String, + pub sha: String, + pub url: String, +} + +#[derive(Debug, serde::Deserialize)] +pub struct Submodule { + pub name: String, + pub path: String, + pub sha: String, + pub submodule_git_url: String, +} + +impl Submodule { + /// Returns the `Repository` this submodule points to. + /// + /// This assumes that the submodule is on GitHub. + pub async fn repository(&self, client: &GithubClient) -> anyhow::Result { + let fullname = self + .submodule_git_url + .strip_prefix("https://github.com/") + .ok_or_else(|| { + anyhow::anyhow!( + "only github submodules supported, got {}", + self.submodule_git_url + ) + })? + .strip_suffix(".git") + .ok_or_else(|| { + anyhow::anyhow!("expected .git suffix, got {}", self.submodule_git_url) + })?; + client.repository(fullname).await + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/handlers.rs b/src/handlers.rs index b948486e..c0cf9dd1 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -27,6 +27,7 @@ mod assign; mod autolabel; mod close; mod decision; +pub mod docs_update; mod github_releases; mod glacier; pub mod jobs; @@ -43,7 +44,7 @@ mod prioritize; mod relabel; mod review_submitted; mod rfc_helper; -mod rustc_commits; +pub mod rustc_commits; mod shortcut; pub async fn handle(ctx: &Context, event: &Event) -> Vec { diff --git a/src/handlers/docs_update.rs b/src/handlers/docs_update.rs new file mode 100644 index 00000000..1c2d0eb2 --- /dev/null +++ b/src/handlers/docs_update.rs @@ -0,0 +1,201 @@ +//! A scheduled job to post a PR to update the documentation on rust-lang/rust. + +use crate::db::jobs::JobSchedule; +use crate::github::{self, GitTreeEntry, GithubClient, Repository}; +use anyhow::Context; +use anyhow::Result; +use cron::Schedule; +use reqwest::Client; +use std::fmt::Write; +use std::str::FromStr; + +/// This is the repository where the commits will be created. +const WORK_REPO: &str = "rustbot/rust"; +/// This is the repository where the PR will be created. +const DEST_REPO: &str = "rust-lang/rust"; +/// This is the branch in `WORK_REPO` to create the commits. +const BRANCH_NAME: &str = "docs-update"; + +const SUBMODULES: &[&str] = &[ + "src/doc/book", + "src/doc/edition-guide", + "src/doc/embedded-book", + "src/doc/nomicon", + "src/doc/reference", + "src/doc/rust-by-example", + "src/doc/rustc-dev-guide", +]; + +const TITLE: &str = "Update books"; + +pub fn job() -> JobSchedule { + JobSchedule { + name: "docs_update".to_string(), + // Around 9am Pacific time on every Monday. + schedule: Schedule::from_str("0 00 17 * * Mon *").unwrap(), + metadata: serde_json::Value::Null, + } +} + +pub async fn handle_job() -> Result<()> { + // Only run every other week. Doing it every week can be a bit noisy, and + // (rarely) a PR can take longer than a week to merge (like if there are + // CI issues). `Schedule` does not allow expressing this, so check it + // manually. + // + // This is set to run the first week after a release, and the week just + // before a release. That allows getting the latest changes in the next + // release, accounting for possibly taking a few days for the PR to land. + let today = chrono::Utc::today().naive_utc(); + let base = chrono::naive::NaiveDate::from_ymd(2015, 12, 10); + let duration = today.signed_duration_since(base); + let weeks = duration.num_weeks(); + if weeks % 2 != 0 { + tracing::trace!("skipping job, this is an odd week"); + return Ok(()); + } + + tracing::trace!("starting docs-update"); + docs_update().await.context("failed to process docs update") +} + +async fn docs_update() -> Result<()> { + let gh = GithubClient::new_with_default_token(Client::new()); + let work_repo = gh.repository(WORK_REPO).await?; + work_repo + .merge_upstream(&gh, &work_repo.default_branch) + .await?; + + let updates = get_submodule_updates(&gh, &work_repo).await?; + if updates.is_empty() { + tracing::trace!("no updates this week?"); + return Ok(()); + } + + create_commit(&gh, &work_repo, &updates).await?; + create_pr(&gh, &updates).await?; + Ok(()) +} + +struct Update { + path: String, + new_hash: String, + pr_body: String, +} + +async fn get_submodule_updates( + gh: &GithubClient, + repo: &github::Repository, +) -> Result> { + let mut updates = Vec::new(); + for submodule_path in SUBMODULES { + tracing::trace!("checking submodule {submodule_path}"); + let submodule = repo.submodule(gh, submodule_path, None).await?; + let submodule_repo = submodule.repository(gh).await?; + let latest_commit = submodule_repo + .get_reference(gh, &format!("heads/{}", submodule_repo.default_branch)) + .await?; + if submodule.sha == latest_commit.object.sha { + tracing::trace!( + "skipping submodule {submodule_path}, no changes sha={}", + submodule.sha + ); + continue; + } + let current_hash = submodule.sha; + let new_hash = latest_commit.object.sha; + let pr_body = generate_pr_body(gh, &submodule_repo, ¤t_hash, &new_hash).await?; + + let update = Update { + path: submodule.path, + new_hash, + pr_body, + }; + updates.push(update); + } + Ok(updates) +} + +async fn generate_pr_body( + gh: &GithubClient, + repo: &github::Repository, + oldest: &str, + newest: &str, +) -> Result { + let recent_commits: Vec<_> = repo + .recent_commits(gh, &repo.default_branch, oldest, newest) + .await?; + if recent_commits.is_empty() { + anyhow::bail!( + "unexpected empty set of commits for {} oldest={oldest} newest={newest}", + repo.full_name + ); + } + let mut body = format!( + "## {}\n\ + \n\ + {} commits in {}..{}\n\ + {} to {}\n\ + \n", + repo.full_name, + recent_commits.len(), + oldest, + newest, + recent_commits.first().unwrap().committed_date, + recent_commits.last().unwrap().committed_date, + ); + for commit in recent_commits { + write!(body, "- {}", commit.title).unwrap(); + if let Some(num) = commit.pr_num { + write!(body, " ({}#{})", repo.full_name, num).unwrap(); + } + body.push('\n'); + } + Ok(body) +} + +async fn create_commit( + gh: &GithubClient, + rust_repo: &Repository, + updates: &[Update], +) -> Result<()> { + let master_ref = rust_repo + .get_reference(gh, &format!("heads/{}", rust_repo.default_branch)) + .await?; + let master_commit = rust_repo.git_commit(gh, &master_ref.object.sha).await?; + let tree_entries: Vec<_> = updates + .iter() + .map(|update| GitTreeEntry { + path: update.path.clone(), + mode: "160000".to_string(), + object_type: "commit".to_string(), + sha: update.new_hash.clone(), + }) + .collect(); + let new_tree = rust_repo + .update_tree(gh, &master_commit.tree.sha, &tree_entries) + .await?; + let commit = rust_repo + .create_commit(gh, TITLE, &[&master_ref.object.sha], &new_tree.sha) + .await?; + rust_repo + .update_reference(gh, &format!("heads/{BRANCH_NAME}"), &commit.sha) + .await?; + Ok(()) +} + +async fn create_pr(gh: &GithubClient, updates: &[Update]) -> Result<()> { + let dest_repo = gh.repository(DEST_REPO).await?; + let mut body = String::new(); + for update in updates { + write!(body, "{}\n", update.pr_body).unwrap(); + } + + let username = WORK_REPO.split('/').next().unwrap(); + let head = format!("{username}:{BRANCH_NAME}"); + let pr = dest_repo + .new_pr(gh, TITLE, &head, &dest_repo.default_branch, &body) + .await?; + tracing::debug!("created PR {}", pr.html_url); + Ok(()) +} diff --git a/src/handlers/jobs.rs b/src/handlers/jobs.rs index 1b111d21..2ab77946 100644 --- a/src/handlers/jobs.rs +++ b/src/handlers/jobs.rs @@ -9,10 +9,20 @@ use crate::handlers::decision::{DecisionProcessActionMetadata, DECISION_PROCESS_ use parser::command::decision::Resolution::Merge; use reqwest::Client; use tracing as log; +use super::Context; -pub async fn handle_job(name: &String, metadata: &serde_json::Value) -> anyhow::Result<()> { - match name { - matched_name if *matched_name == DECISION_PROCESS_JOB_NAME.to_string() => { +pub async fn handle_job( + ctx: &Context, + name: &String, + metadata: &serde_json::Value, +) -> anyhow::Result<()> { + match name.as_str() { + "docs_update" => super::docs_update::handle_job().await, + "rustc_commits" => { + super::rustc_commits::synchronize_commits_inner(ctx, None).await; + Ok(()) + }, + DECISION_PROCESS_JOB_NAME => { decision_process_handler(&metadata).await } _ => default(&name, &metadata), diff --git a/src/handlers/rustc_commits.rs b/src/handlers/rustc_commits.rs index e107a1fd..4724bb2b 100644 --- a/src/handlers/rustc_commits.rs +++ b/src/handlers/rustc_commits.rs @@ -1,11 +1,14 @@ +use crate::db::jobs::JobSchedule; use crate::db::rustc_commits; use crate::db::rustc_commits::get_missing_commits; use crate::{ github::{self, Event}, handlers::Context, }; +use cron::Schedule; use std::collections::VecDeque; use std::convert::TryInto; +use std::str::FromStr; use tracing as log; const BORS_GH_ID: i64 = 3372342; @@ -80,16 +83,28 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> { /// Fetch commits that are not present in the database. async fn synchronize_commits(ctx: &Context, sha: &str, pr: u32) { log::trace!("synchronize_commits for sha={:?}, pr={}", sha, pr); + synchronize_commits_inner(ctx, Some((sha.to_owned(), pr))).await; +} + +pub async fn synchronize_commits_inner(ctx: &Context, starter: Option<(String, u32)>) { let db = ctx.db.get().await; - let mut pr = Some(pr); // List of roots to be resolved. Each root and its parents will be recursively resolved // until an existing commit is found. let mut to_be_resolved = VecDeque::new(); - to_be_resolved.push_back(sha.to_string()); - to_be_resolved.extend(get_missing_commits(&db).await); + if let Some((sha, pr)) = starter { + to_be_resolved.push_back((sha.to_string(), Some(pr))); + } + to_be_resolved.extend( + get_missing_commits(&db) + .await + .into_iter() + .map(|c| (c, None::)), + ); + log::info!("synchronize_commits for {:?}", to_be_resolved); - while let Some(sha) = to_be_resolved.pop_front() { + let db = ctx.db.get().await; + while let Some((sha, mut pr)) = to_be_resolved.pop_front() { let mut gc = match ctx.github.rust_commit(&sha).await { Some(c) => c, None => { @@ -130,7 +145,7 @@ async fn synchronize_commits(ctx: &Context, sha: &str, pr: u32) { match res { Ok(()) => { if !rustc_commits::has_commit(&db, &parent_sha).await { - to_be_resolved.push_back(parent_sha) + to_be_resolved.push_back((parent_sha, None)) } } Err(e) => log::error!("Failed to record commit {:?}", e), @@ -138,6 +153,15 @@ async fn synchronize_commits(ctx: &Context, sha: &str, pr: u32) { } } +pub fn job() -> JobSchedule { + JobSchedule { + name: "rustc_commits".to_string(), + // Every 30 minutes... + schedule: Schedule::from_str("* 0,30 * * * * *").unwrap(), + metadata: serde_json::Value::Null, + } +} + #[derive(Debug, serde::Deserialize)] struct BorsMessage { #[serde(rename = "type")] diff --git a/src/jobs.rs b/src/jobs.rs index a680f25c..3ba6de49 100644 --- a/src/jobs.rs +++ b/src/jobs.rs @@ -45,7 +45,15 @@ pub const JOB_PROCESSING_CADENCE_IN_SECS: u64 = 60; pub fn jobs() -> Vec { // Add to this vector any new cron task you want (as explained above) - let jobs: Vec = Vec::new(); + let mut jobs: Vec = Vec::new(); + jobs.push(crate::handlers::docs_update::job()); + jobs.push(crate::handlers::rustc_commits::job()); jobs } + +#[test] +fn jobs_defined() { + // Checks we don't panic here, mostly for the schedule parsing. + drop(jobs()); +} diff --git a/src/main.rs b/src/main.rs index 24c124bf..d119e3c9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -267,10 +267,25 @@ async fn run_server(addr: SocketAddr) -> anyhow::Result<()> { } }); + let client = Client::new(); + let gh = github::GithubClient::new_with_default_token(client.clone()); + let oc = octocrab::OctocrabBuilder::new() + .personal_token(github::default_token_from_env()) + .build() + .expect("Failed to build octograb."); + let ctx = Arc::new(Context { + username: String::from("rustbot"), + db: pool, + github: gh, + octocrab: oc, + }); + // spawning a background task that will run the scheduled jobs // every JOB_PROCESSING_CADENCE_IN_SECS + let ctx2 = ctx.clone(); task::spawn(async move { loop { + let ctx = ctx2.clone(); let res = task::spawn(async move { let pool = db::ClientPool::new(); let mut interval = @@ -278,7 +293,7 @@ async fn run_server(addr: SocketAddr) -> anyhow::Result<()> { loop { interval.tick().await; - db::run_scheduled_jobs(&*pool.get().await) + db::run_scheduled_jobs(&ctx, &*pool.get().await) .await .context("run database scheduled jobs") .unwrap(); @@ -295,19 +310,6 @@ async fn run_server(addr: SocketAddr) -> anyhow::Result<()> { } }); - let client = Client::new(); - let gh = github::GithubClient::new_with_default_token(client.clone()); - let oc = octocrab::OctocrabBuilder::new() - .personal_token(github::default_token_from_env()) - .build() - .expect("Failed to build octograb."); - let ctx = Arc::new(Context { - username: String::from("rustbot"), - db: pool, - github: gh, - octocrab: oc, - }); - let agenda = tower::ServiceBuilder::new() .buffer(10) .layer_fn(|input| { From fae981d3818ae044abfdb0d24d5a222398e3b8ab Mon Sep 17 00:00:00 2001 From: Julian Montes de Oca Date: Mon, 12 Dec 2022 09:12:07 -0300 Subject: [PATCH 11/15] Revert "Merge master" This reverts commit 6fd8baab6312b0e83beb489843c2941cf2832fc0. --- github-graphql/src/lib.rs | 151 +----------- src/db.rs | 8 +- src/github.rs | 430 +--------------------------------- src/handlers.rs | 3 +- src/handlers/docs_update.rs | 201 ---------------- src/handlers/jobs.rs | 16 +- src/handlers/rustc_commits.rs | 34 +-- src/jobs.rs | 10 +- src/main.rs | 30 ++- 9 files changed, 31 insertions(+), 852 deletions(-) delete mode 100644 src/handlers/docs_update.rs diff --git a/github-graphql/src/lib.rs b/github-graphql/src/lib.rs index 6ccf10e7..b508dcb0 100644 --- a/github-graphql/src/lib.rs +++ b/github-graphql/src/lib.rs @@ -2,7 +2,7 @@ //! //! See for more GitHub's GraphQL API. -// This schema can be downloaded from https://docs.github.com/public/schema.docs.graphql +// This schema can be downloaded from https://docs.github.com/en/graphql/overview/public-schema #[cynic::schema_for_derives(file = "src/github.graphql", module = "schema")] pub mod queries { use super::schema; @@ -155,155 +155,6 @@ pub mod queries { pub struct Uri(pub String); } -#[cynic::schema_for_derives(file = "src/github.graphql", module = "schema")] -pub mod docs_update_queries { - use super::queries::{DateTime, PageInfo}; - use super::schema; - - #[derive(cynic::FragmentArguments, Debug)] - pub struct RecentCommitsArguments { - pub branch: String, - pub name: String, - pub owner: String, - pub after: Option, - } - - /// Query for fetching recent commits and their associated PRs. - /// - /// This query is built from: - /// - /// ```text - /// query RecentCommits($name: String!, $owner: String!, $branch: String!, $after: String) { - /// repository(name: $name, owner: $owner) { - /// ref(qualifiedName: $branch) { - /// target { - /// ... on Commit { - /// history(first: 100, after: $after) { - /// totalCount - /// pageInfo { - /// hasNextPage - /// endCursor - /// } - /// nodes { - /// oid - /// parents(first: 1) { - /// nodes { - /// oid - /// } - /// } - /// committedDate - /// messageHeadline - /// associatedPullRequests(first: 1) { - /// nodes { - /// number - /// title - /// } - /// } - /// } - /// } - /// } - /// } - /// } - /// } - /// } - /// ``` - #[derive(cynic::QueryFragment, Debug)] - #[cynic(graphql_type = "Query", argument_struct = "RecentCommitsArguments")] - pub struct RecentCommits { - #[arguments(name = &args.name, owner = &args.owner)] - pub repository: Option, - } - - #[derive(cynic::QueryFragment, Debug)] - #[cynic(argument_struct = "RecentCommitsArguments")] - pub struct Repository { - #[arguments(qualified_name = &args.branch)] - #[cynic(rename = "ref")] - pub ref_: Option, - } - - #[derive(cynic::QueryFragment, Debug)] - #[cynic(argument_struct = "RecentCommitsArguments")] - pub struct Ref { - pub target: Option, - } - - #[derive(cynic::QueryFragment, Debug)] - #[cynic(argument_struct = "RecentCommitsArguments")] - pub struct Commit { - #[arguments(first = 100, after = &args.after)] - pub history: CommitHistoryConnection, - } - - #[derive(cynic::QueryFragment, Debug)] - pub struct CommitHistoryConnection { - pub total_count: i32, - pub page_info: PageInfo, - pub nodes: Option>>, - } - - #[derive(cynic::QueryFragment, Debug)] - #[cynic(graphql_type = "Commit")] - pub struct Commit2 { - pub oid: GitObjectID, - #[arguments(first = 1)] - pub parents: CommitConnection, - pub committed_date: DateTime, - pub message_headline: String, - #[arguments(first = 1)] - pub associated_pull_requests: Option, - } - - #[derive(cynic::QueryFragment, Debug)] - pub struct PullRequestConnection { - pub nodes: Option>>, - } - - #[derive(cynic::QueryFragment, Debug)] - pub struct PullRequest { - pub number: i32, - pub title: String, - } - - #[derive(cynic::QueryFragment, Debug)] - pub struct CommitConnection { - pub nodes: Option>>, - } - - #[derive(cynic::QueryFragment, Debug)] - #[cynic(graphql_type = "Commit")] - pub struct Commit3 { - pub oid: GitObjectID, - } - - #[derive(cynic::InlineFragments, Debug)] - #[cynic(argument_struct = "RecentCommitsArguments")] - pub enum GitObject { - Commit(Commit), - // These three variants are here just to pacify cynic. I don't know - // why it fails to compile without them. - Tree(Tree), - Tag(Tag), - Blob(Blob), - } - - #[derive(cynic::QueryFragment, Debug)] - pub struct Tree { - pub id: cynic::Id, - } - #[derive(cynic::QueryFragment, Debug)] - pub struct Tag { - pub id: cynic::Id, - } - #[derive(cynic::QueryFragment, Debug)] - pub struct Blob { - pub id: cynic::Id, - } - - #[derive(cynic::Scalar, Debug, Clone)] - pub struct GitObjectID(pub String); -} - mod schema { cynic::use_schema!("src/github.graphql"); } diff --git a/src/db.rs b/src/db.rs index bd3de5cc..1a83a8bf 100644 --- a/src/db.rs +++ b/src/db.rs @@ -1,5 +1,5 @@ +use crate::db::jobs::*; use crate::handlers::jobs::handle_job; -use crate::{db::jobs::*, handlers::Context}; use anyhow::Context as _; use chrono::Utc; use native_tls::{Certificate, TlsConnector}; @@ -199,20 +199,20 @@ pub async fn schedule_jobs(db: &DbClient, jobs: Vec) -> anyhow::Res Ok(()) } -pub async fn run_scheduled_jobs(ctx: &Context, db: &DbClient) -> anyhow::Result<()> { +pub async fn run_scheduled_jobs(db: &DbClient) -> anyhow::Result<()> { let jobs = get_jobs_to_execute(&db).await.unwrap(); tracing::trace!("jobs to execute: {:#?}", jobs); for job in jobs.iter() { update_job_executed_at(&db, &job.id).await?; - match handle_job(&ctx, &job.name, &job.metadata).await { + match handle_job(&job.name, &job.metadata).await { Ok(_) => { tracing::trace!("job successfully executed (id={})", job.id); delete_job(&db, &job.id).await?; } Err(e) => { - tracing::error!("job failed on execution (id={:?}, error={:?})", job.id, e); + tracing::trace!("job failed on execution (id={:?}, error={:?})", job.id, e); update_job_error_message(&db, &job.id, &e.to_string()).await?; } } diff --git a/src/github.rs b/src/github.rs index 40cc1305..7c1820c8 100644 --- a/src/github.rs +++ b/src/github.rs @@ -6,7 +6,7 @@ use hyper::header::HeaderValue; use once_cell::sync::OnceCell; use reqwest::header::{AUTHORIZATION, USER_AGENT}; use reqwest::{Client, Request, RequestBuilder, Response, StatusCode}; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::convert::TryInto; use std::{ fmt, @@ -1028,10 +1028,6 @@ impl Repository { const GITHUB_API_URL: &'static str = "https://api.github.com"; const GITHUB_GRAPHQL_API_URL: &'static str = "https://api.github.com/graphql"; - fn url(&self) -> String { - format!("{}/repos/{}", Repository::GITHUB_API_URL, self.full_name) - } - pub fn owner(&self) -> &str { self.full_name.split_once('/').unwrap().0 } @@ -1205,338 +1201,6 @@ impl Repository { ordering.page, ) } - - /// Retrieves a git commit for the given SHA. - pub async fn git_commit(&self, client: &GithubClient, sha: &str) -> anyhow::Result { - let url = format!("{}/git/commits/{sha}", self.url()); - client - .json(client.get(&url)) - .await - .with_context(|| format!("{} failed to get git commit {sha}", self.full_name)) - } - - /// Creates a new commit. - pub async fn create_commit( - &self, - client: &GithubClient, - message: &str, - parents: &[&str], - tree: &str, - ) -> anyhow::Result { - let url = format!("{}/git/commits", self.url()); - client - .json(client.post(&url).json(&serde_json::json!({ - "message": message, - "parents": parents, - "tree": tree, - }))) - .await - .with_context(|| format!("{} failed to create commit for tree {tree}", self.full_name)) - } - - /// Retrieves a git reference for the given refname. - pub async fn get_reference( - &self, - client: &GithubClient, - refname: &str, - ) -> anyhow::Result { - let url = format!("{}/git/ref/{}", self.url(), refname); - client - .json(client.get(&url)) - .await - .with_context(|| format!("{} failed to get git reference {refname}", self.full_name)) - } - - /// Updates an existing git reference to a new SHA. - pub async fn update_reference( - &self, - client: &GithubClient, - refname: &str, - sha: &str, - ) -> anyhow::Result<()> { - let url = format!("{}/git/refs/{}", self.url(), refname); - client - ._send_req(client.patch(&url).json(&serde_json::json!({ - "sha": sha, - "force": true, - }))) - .await - .with_context(|| { - format!( - "{} failed to update reference {refname} to {sha}", - self.full_name - ) - })?; - Ok(()) - } - - /// Returns a list of recent commits on the given branch. - /// - /// Returns results in the OID range `oldest` (exclusive) to `newest` - /// (inclusive). - pub async fn recent_commits( - &self, - client: &GithubClient, - branch: &str, - oldest: &str, - newest: &str, - ) -> anyhow::Result> { - // This is used to deduplicate the results (so that a PR with multiple - // commits will only show up once). - let mut prs_seen = HashSet::new(); - let mut recent_commits = Vec::new(); // This is the final result. - use cynic::QueryBuilder; - use github_graphql::docs_update_queries::{ - GitObject, RecentCommits, RecentCommitsArguments, - }; - - let mut args = RecentCommitsArguments { - branch: branch.to_string(), - name: self.name().to_string(), - owner: self.owner().to_string(), - after: None, - }; - let mut found_newest = false; - let mut found_oldest = false; - // This simulates --first-parent. We only care about top-level commits. - // Unfortunately the GitHub API doesn't provide anything like that. - let mut next_first_parent = None; - // Search for `oldest` within 3 pages (300 commits). - for _ in 0..3 { - let query = RecentCommits::build(&args); - let response = client - .json(client.post(Repository::GITHUB_GRAPHQL_API_URL).json(&query)) - .await - .with_context(|| { - format!( - "{} failed to get recent commits branch={branch}", - self.full_name - ) - })?; - let data: cynic::GraphQlResponse = query - .decode_response(response) - .with_context(|| format!("failed to parse response for `RecentCommits`"))?; - if let Some(errors) = data.errors { - anyhow::bail!("There were graphql errors. {:?}", errors); - } - let target = data - .data - .ok_or_else(|| anyhow::anyhow!("No data returned."))? - .repository - .ok_or_else(|| anyhow::anyhow!("No repository."))? - .ref_ - .ok_or_else(|| anyhow::anyhow!("No ref."))? - .target - .ok_or_else(|| anyhow::anyhow!("No target."))?; - let commit = match target { - GitObject::Commit(commit) => commit, - _ => anyhow::bail!("unexpected target type {target:?}"), - }; - let commits = commit - .history - .nodes - .ok_or_else(|| anyhow::anyhow!("No history."))? - .into_iter() - .filter_map(|node| node) - // Don't include anything newer than `newest` - .skip_while(|node| { - if found_newest || node.oid.0 == newest { - found_newest = true; - false - } else { - // This should only happen if there is a commit that arrives - // between the time that `update_submodules` fetches the latest - // ref, and this runs. This window should be a few seconds, so it - // should be unlikely. This warning is here in case my assumptions - // about how things work is not correct. - tracing::warn!( - "unexpected race with submodule history, newest oid={newest} skipping oid={}", - node.oid.0 - ); - true - } - }) - // Skip nodes that aren't the first parent - .filter(|node| { - let this_first_parent = node.parents.nodes - .as_ref() - // Grab the first parent - .and_then(|nodes| nodes.first()) - // Strip away the useless Option - .and_then(|parent_opt| parent_opt.as_ref()) - .map(|parent| parent.oid.0.clone()); - - match &next_first_parent { - Some(first_parent) => { - if first_parent == &node.oid.0 { - // Found the next first parent, include it and - // set next_first_parent to look for this - // commit's first parent. - next_first_parent = this_first_parent; - true - } else { - // Still looking for the next first parent. - false - } - } - None => { - // First commit. - next_first_parent = this_first_parent; - true - } - } - }) - // Stop once reached the `oldest` commit - .take_while(|node| { - if node.oid.0 == oldest { - found_oldest = true; - false - } else { - true - } - }) - .filter_map(|node| { - // Determine if this is associated with a PR or not. - match node.associated_pull_requests - // Strip away the useless Option - .and_then(|pr| pr.nodes) - // Get the first PR (we only care about one) - .and_then(|mut nodes| nodes.pop()) - // Strip away the useless Option - .flatten() { - Some(pr) => { - // Only include a PR once - if prs_seen.insert(pr.number) { - Some(RecentCommit { - pr_num: Some(pr.number), - title: pr.title, - oid: node.oid.0.clone(), - committed_date: node.committed_date, - }) - } else { - None - } - } - None => { - // This is an unassociated commit, possibly - // created without a PR. - Some(RecentCommit { - pr_num: None, - title: node.message_headline, - oid: node.oid.0, - committed_date: node.committed_date, - }) - } - } - }); - recent_commits.extend(commits); - let page_info = commit.history.page_info; - if found_oldest || !page_info.has_next_page || page_info.end_cursor.is_none() { - break; - } - args.after = page_info.end_cursor; - } - if !found_oldest { - // This should probably do something more than log a warning, but - // I don't think it is too important at this time (the log message - // is only informational, and this should be unlikely to happen). - tracing::warn!( - "{} failed to find oldest commit sha={oldest} branch={branch}", - self.full_name - ); - } - Ok(recent_commits) - } - - /// Creates a new git tree based on another tree. - pub async fn update_tree( - &self, - client: &GithubClient, - base_tree: &str, - tree: &[GitTreeEntry], - ) -> anyhow::Result { - let url = format!("{}/git/trees", self.url()); - client - .json(client.post(&url).json(&serde_json::json!({ - "base_tree": base_tree, - "tree": tree, - }))) - .await - .with_context(|| { - format!( - "{} failed to update tree with base {base_tree}", - self.full_name - ) - }) - } - - /// Returns information about the git submodule at the given path. - /// - /// `refname` is the ref to use for fetching information. If `None`, will - /// use the latest version on the default branch. - pub async fn submodule( - &self, - client: &GithubClient, - path: &str, - refname: Option<&str>, - ) -> anyhow::Result { - let mut url = format!("{}/contents/{}", self.url(), path); - if let Some(refname) = refname { - url.push_str("?ref="); - url.push_str(refname); - } - client.json(client.get(&url)).await.with_context(|| { - format!( - "{} failed to get submodule path={path} refname={refname:?}", - self.full_name - ) - }) - } - - /// Creates a new PR. - pub async fn new_pr( - &self, - client: &GithubClient, - title: &str, - head: &str, - base: &str, - body: &str, - ) -> anyhow::Result { - let url = format!("{}/pulls", self.url()); - let mut issue: Issue = client - .json(client.post(&url).json(&serde_json::json!({ - "title": title, - "head": head, - "base": base, - "body": body, - }))) - .await - .with_context(|| { - format!( - "{} failed to create a new PR head={head} base={base} title={title}", - self.full_name - ) - })?; - issue.pull_request = Some(PullRequestDetails {}); - Ok(issue) - } - - /// Synchronize a branch (in a forked repository) by pulling in its upstream contents. - pub async fn merge_upstream(&self, client: &GithubClient, branch: &str) -> anyhow::Result<()> { - let url = format!("{}/merge-upstream", self.url()); - client - ._send_req(client.post(&url).json(&serde_json::json!({ - "branch": branch, - }))) - .await - .with_context(|| { - format!( - "{} failed to merge upstream branch {branch}", - self.full_name - ) - })?; - Ok(()) - } } pub struct Query<'a> { @@ -1934,64 +1598,19 @@ impl GithubClient { } } } - - /// Returns information about a repository. - /// - /// The `full_name` should be something like `rust-lang/rust`. - pub async fn repository(&self, full_name: &str) -> anyhow::Result { - let req = self.get(&format!("{}/repos/{full_name}", Repository::GITHUB_API_URL)); - self.json(req) - .await - .with_context(|| format!("{} failed to get repo", full_name)) - } } #[derive(Debug, serde::Deserialize)] pub struct GithubCommit { pub sha: String, - pub commit: GithubCommitCommitField, + pub commit: GitCommit, pub parents: Vec, } -#[derive(Debug, serde::Deserialize)] -pub struct GithubCommitCommitField { - pub author: GitUser, - pub message: String, - pub tree: GitCommitTree, -} - #[derive(Debug, serde::Deserialize)] pub struct GitCommit { - pub sha: String, pub author: GitUser, pub message: String, - pub tree: GitCommitTree, -} - -#[derive(Debug, serde::Deserialize)] -pub struct GitCommitTree { - pub sha: String, -} - -#[derive(Debug, serde::Deserialize)] -pub struct GitTreeObject { - pub sha: String, -} - -#[derive(Debug, serde::Serialize, serde::Deserialize)] -pub struct GitTreeEntry { - pub path: String, - pub mode: String, - #[serde(rename = "type")] - pub object_type: String, - pub sha: String, -} - -pub struct RecentCommit { - pub title: String, - pub pr_num: Option, - pub oid: String, - pub committed_date: DateTime, } #[derive(Debug, serde::Deserialize)] @@ -2185,51 +1804,6 @@ impl IssuesQuery for LeastRecentlyReviewedPullRequests { } } -#[derive(Debug, serde::Deserialize)] -pub struct GitReference { - #[serde(rename = "ref")] - pub refname: String, - pub object: GitObject, -} - -#[derive(Debug, serde::Deserialize)] -pub struct GitObject { - #[serde(rename = "type")] - pub object_type: String, - pub sha: String, - pub url: String, -} - -#[derive(Debug, serde::Deserialize)] -pub struct Submodule { - pub name: String, - pub path: String, - pub sha: String, - pub submodule_git_url: String, -} - -impl Submodule { - /// Returns the `Repository` this submodule points to. - /// - /// This assumes that the submodule is on GitHub. - pub async fn repository(&self, client: &GithubClient) -> anyhow::Result { - let fullname = self - .submodule_git_url - .strip_prefix("https://github.com/") - .ok_or_else(|| { - anyhow::anyhow!( - "only github submodules supported, got {}", - self.submodule_git_url - ) - })? - .strip_suffix(".git") - .ok_or_else(|| { - anyhow::anyhow!("expected .git suffix, got {}", self.submodule_git_url) - })?; - client.repository(fullname).await - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/handlers.rs b/src/handlers.rs index c0cf9dd1..b948486e 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -27,7 +27,6 @@ mod assign; mod autolabel; mod close; mod decision; -pub mod docs_update; mod github_releases; mod glacier; pub mod jobs; @@ -44,7 +43,7 @@ mod prioritize; mod relabel; mod review_submitted; mod rfc_helper; -pub mod rustc_commits; +mod rustc_commits; mod shortcut; pub async fn handle(ctx: &Context, event: &Event) -> Vec { diff --git a/src/handlers/docs_update.rs b/src/handlers/docs_update.rs deleted file mode 100644 index 1c2d0eb2..00000000 --- a/src/handlers/docs_update.rs +++ /dev/null @@ -1,201 +0,0 @@ -//! A scheduled job to post a PR to update the documentation on rust-lang/rust. - -use crate::db::jobs::JobSchedule; -use crate::github::{self, GitTreeEntry, GithubClient, Repository}; -use anyhow::Context; -use anyhow::Result; -use cron::Schedule; -use reqwest::Client; -use std::fmt::Write; -use std::str::FromStr; - -/// This is the repository where the commits will be created. -const WORK_REPO: &str = "rustbot/rust"; -/// This is the repository where the PR will be created. -const DEST_REPO: &str = "rust-lang/rust"; -/// This is the branch in `WORK_REPO` to create the commits. -const BRANCH_NAME: &str = "docs-update"; - -const SUBMODULES: &[&str] = &[ - "src/doc/book", - "src/doc/edition-guide", - "src/doc/embedded-book", - "src/doc/nomicon", - "src/doc/reference", - "src/doc/rust-by-example", - "src/doc/rustc-dev-guide", -]; - -const TITLE: &str = "Update books"; - -pub fn job() -> JobSchedule { - JobSchedule { - name: "docs_update".to_string(), - // Around 9am Pacific time on every Monday. - schedule: Schedule::from_str("0 00 17 * * Mon *").unwrap(), - metadata: serde_json::Value::Null, - } -} - -pub async fn handle_job() -> Result<()> { - // Only run every other week. Doing it every week can be a bit noisy, and - // (rarely) a PR can take longer than a week to merge (like if there are - // CI issues). `Schedule` does not allow expressing this, so check it - // manually. - // - // This is set to run the first week after a release, and the week just - // before a release. That allows getting the latest changes in the next - // release, accounting for possibly taking a few days for the PR to land. - let today = chrono::Utc::today().naive_utc(); - let base = chrono::naive::NaiveDate::from_ymd(2015, 12, 10); - let duration = today.signed_duration_since(base); - let weeks = duration.num_weeks(); - if weeks % 2 != 0 { - tracing::trace!("skipping job, this is an odd week"); - return Ok(()); - } - - tracing::trace!("starting docs-update"); - docs_update().await.context("failed to process docs update") -} - -async fn docs_update() -> Result<()> { - let gh = GithubClient::new_with_default_token(Client::new()); - let work_repo = gh.repository(WORK_REPO).await?; - work_repo - .merge_upstream(&gh, &work_repo.default_branch) - .await?; - - let updates = get_submodule_updates(&gh, &work_repo).await?; - if updates.is_empty() { - tracing::trace!("no updates this week?"); - return Ok(()); - } - - create_commit(&gh, &work_repo, &updates).await?; - create_pr(&gh, &updates).await?; - Ok(()) -} - -struct Update { - path: String, - new_hash: String, - pr_body: String, -} - -async fn get_submodule_updates( - gh: &GithubClient, - repo: &github::Repository, -) -> Result> { - let mut updates = Vec::new(); - for submodule_path in SUBMODULES { - tracing::trace!("checking submodule {submodule_path}"); - let submodule = repo.submodule(gh, submodule_path, None).await?; - let submodule_repo = submodule.repository(gh).await?; - let latest_commit = submodule_repo - .get_reference(gh, &format!("heads/{}", submodule_repo.default_branch)) - .await?; - if submodule.sha == latest_commit.object.sha { - tracing::trace!( - "skipping submodule {submodule_path}, no changes sha={}", - submodule.sha - ); - continue; - } - let current_hash = submodule.sha; - let new_hash = latest_commit.object.sha; - let pr_body = generate_pr_body(gh, &submodule_repo, ¤t_hash, &new_hash).await?; - - let update = Update { - path: submodule.path, - new_hash, - pr_body, - }; - updates.push(update); - } - Ok(updates) -} - -async fn generate_pr_body( - gh: &GithubClient, - repo: &github::Repository, - oldest: &str, - newest: &str, -) -> Result { - let recent_commits: Vec<_> = repo - .recent_commits(gh, &repo.default_branch, oldest, newest) - .await?; - if recent_commits.is_empty() { - anyhow::bail!( - "unexpected empty set of commits for {} oldest={oldest} newest={newest}", - repo.full_name - ); - } - let mut body = format!( - "## {}\n\ - \n\ - {} commits in {}..{}\n\ - {} to {}\n\ - \n", - repo.full_name, - recent_commits.len(), - oldest, - newest, - recent_commits.first().unwrap().committed_date, - recent_commits.last().unwrap().committed_date, - ); - for commit in recent_commits { - write!(body, "- {}", commit.title).unwrap(); - if let Some(num) = commit.pr_num { - write!(body, " ({}#{})", repo.full_name, num).unwrap(); - } - body.push('\n'); - } - Ok(body) -} - -async fn create_commit( - gh: &GithubClient, - rust_repo: &Repository, - updates: &[Update], -) -> Result<()> { - let master_ref = rust_repo - .get_reference(gh, &format!("heads/{}", rust_repo.default_branch)) - .await?; - let master_commit = rust_repo.git_commit(gh, &master_ref.object.sha).await?; - let tree_entries: Vec<_> = updates - .iter() - .map(|update| GitTreeEntry { - path: update.path.clone(), - mode: "160000".to_string(), - object_type: "commit".to_string(), - sha: update.new_hash.clone(), - }) - .collect(); - let new_tree = rust_repo - .update_tree(gh, &master_commit.tree.sha, &tree_entries) - .await?; - let commit = rust_repo - .create_commit(gh, TITLE, &[&master_ref.object.sha], &new_tree.sha) - .await?; - rust_repo - .update_reference(gh, &format!("heads/{BRANCH_NAME}"), &commit.sha) - .await?; - Ok(()) -} - -async fn create_pr(gh: &GithubClient, updates: &[Update]) -> Result<()> { - let dest_repo = gh.repository(DEST_REPO).await?; - let mut body = String::new(); - for update in updates { - write!(body, "{}\n", update.pr_body).unwrap(); - } - - let username = WORK_REPO.split('/').next().unwrap(); - let head = format!("{username}:{BRANCH_NAME}"); - let pr = dest_repo - .new_pr(gh, TITLE, &head, &dest_repo.default_branch, &body) - .await?; - tracing::debug!("created PR {}", pr.html_url); - Ok(()) -} diff --git a/src/handlers/jobs.rs b/src/handlers/jobs.rs index 2ab77946..1b111d21 100644 --- a/src/handlers/jobs.rs +++ b/src/handlers/jobs.rs @@ -9,20 +9,10 @@ use crate::handlers::decision::{DecisionProcessActionMetadata, DECISION_PROCESS_ use parser::command::decision::Resolution::Merge; use reqwest::Client; use tracing as log; -use super::Context; -pub async fn handle_job( - ctx: &Context, - name: &String, - metadata: &serde_json::Value, -) -> anyhow::Result<()> { - match name.as_str() { - "docs_update" => super::docs_update::handle_job().await, - "rustc_commits" => { - super::rustc_commits::synchronize_commits_inner(ctx, None).await; - Ok(()) - }, - DECISION_PROCESS_JOB_NAME => { +pub async fn handle_job(name: &String, metadata: &serde_json::Value) -> anyhow::Result<()> { + match name { + matched_name if *matched_name == DECISION_PROCESS_JOB_NAME.to_string() => { decision_process_handler(&metadata).await } _ => default(&name, &metadata), diff --git a/src/handlers/rustc_commits.rs b/src/handlers/rustc_commits.rs index 4724bb2b..e107a1fd 100644 --- a/src/handlers/rustc_commits.rs +++ b/src/handlers/rustc_commits.rs @@ -1,14 +1,11 @@ -use crate::db::jobs::JobSchedule; use crate::db::rustc_commits; use crate::db::rustc_commits::get_missing_commits; use crate::{ github::{self, Event}, handlers::Context, }; -use cron::Schedule; use std::collections::VecDeque; use std::convert::TryInto; -use std::str::FromStr; use tracing as log; const BORS_GH_ID: i64 = 3372342; @@ -83,28 +80,16 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> { /// Fetch commits that are not present in the database. async fn synchronize_commits(ctx: &Context, sha: &str, pr: u32) { log::trace!("synchronize_commits for sha={:?}, pr={}", sha, pr); - synchronize_commits_inner(ctx, Some((sha.to_owned(), pr))).await; -} - -pub async fn synchronize_commits_inner(ctx: &Context, starter: Option<(String, u32)>) { let db = ctx.db.get().await; + let mut pr = Some(pr); // List of roots to be resolved. Each root and its parents will be recursively resolved // until an existing commit is found. let mut to_be_resolved = VecDeque::new(); - if let Some((sha, pr)) = starter { - to_be_resolved.push_back((sha.to_string(), Some(pr))); - } - to_be_resolved.extend( - get_missing_commits(&db) - .await - .into_iter() - .map(|c| (c, None::)), - ); - log::info!("synchronize_commits for {:?}", to_be_resolved); + to_be_resolved.push_back(sha.to_string()); + to_be_resolved.extend(get_missing_commits(&db).await); - let db = ctx.db.get().await; - while let Some((sha, mut pr)) = to_be_resolved.pop_front() { + while let Some(sha) = to_be_resolved.pop_front() { let mut gc = match ctx.github.rust_commit(&sha).await { Some(c) => c, None => { @@ -145,7 +130,7 @@ pub async fn synchronize_commits_inner(ctx: &Context, starter: Option<(String, u match res { Ok(()) => { if !rustc_commits::has_commit(&db, &parent_sha).await { - to_be_resolved.push_back((parent_sha, None)) + to_be_resolved.push_back(parent_sha) } } Err(e) => log::error!("Failed to record commit {:?}", e), @@ -153,15 +138,6 @@ pub async fn synchronize_commits_inner(ctx: &Context, starter: Option<(String, u } } -pub fn job() -> JobSchedule { - JobSchedule { - name: "rustc_commits".to_string(), - // Every 30 minutes... - schedule: Schedule::from_str("* 0,30 * * * * *").unwrap(), - metadata: serde_json::Value::Null, - } -} - #[derive(Debug, serde::Deserialize)] struct BorsMessage { #[serde(rename = "type")] diff --git a/src/jobs.rs b/src/jobs.rs index 3ba6de49..a680f25c 100644 --- a/src/jobs.rs +++ b/src/jobs.rs @@ -45,15 +45,7 @@ pub const JOB_PROCESSING_CADENCE_IN_SECS: u64 = 60; pub fn jobs() -> Vec { // Add to this vector any new cron task you want (as explained above) - let mut jobs: Vec = Vec::new(); - jobs.push(crate::handlers::docs_update::job()); - jobs.push(crate::handlers::rustc_commits::job()); + let jobs: Vec = Vec::new(); jobs } - -#[test] -fn jobs_defined() { - // Checks we don't panic here, mostly for the schedule parsing. - drop(jobs()); -} diff --git a/src/main.rs b/src/main.rs index d119e3c9..24c124bf 100644 --- a/src/main.rs +++ b/src/main.rs @@ -267,25 +267,10 @@ async fn run_server(addr: SocketAddr) -> anyhow::Result<()> { } }); - let client = Client::new(); - let gh = github::GithubClient::new_with_default_token(client.clone()); - let oc = octocrab::OctocrabBuilder::new() - .personal_token(github::default_token_from_env()) - .build() - .expect("Failed to build octograb."); - let ctx = Arc::new(Context { - username: String::from("rustbot"), - db: pool, - github: gh, - octocrab: oc, - }); - // spawning a background task that will run the scheduled jobs // every JOB_PROCESSING_CADENCE_IN_SECS - let ctx2 = ctx.clone(); task::spawn(async move { loop { - let ctx = ctx2.clone(); let res = task::spawn(async move { let pool = db::ClientPool::new(); let mut interval = @@ -293,7 +278,7 @@ async fn run_server(addr: SocketAddr) -> anyhow::Result<()> { loop { interval.tick().await; - db::run_scheduled_jobs(&ctx, &*pool.get().await) + db::run_scheduled_jobs(&*pool.get().await) .await .context("run database scheduled jobs") .unwrap(); @@ -310,6 +295,19 @@ async fn run_server(addr: SocketAddr) -> anyhow::Result<()> { } }); + let client = Client::new(); + let gh = github::GithubClient::new_with_default_token(client.clone()); + let oc = octocrab::OctocrabBuilder::new() + .personal_token(github::default_token_from_env()) + .build() + .expect("Failed to build octograb."); + let ctx = Arc::new(Context { + username: String::from("rustbot"), + db: pool, + github: gh, + octocrab: oc, + }); + let agenda = tower::ServiceBuilder::new() .buffer(10) .layer_fn(|input| { From 6f581cf43ffda51e3f7af4d82c70bf62d0e44a97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Montes=20de=20Oca?= Date: Tue, 3 Jan 2023 16:34:40 -0300 Subject: [PATCH 12/15] Decision state comment builder * add comment builder for the decision state * improve error handling * add factory for user status and add tests * cleanup unnecessary code --- Cargo.lock | 21 ++++ Cargo.toml | 1 + parser/src/command/decision.rs | 10 ++ src/db/issue_decision_state.rs | 7 +- src/handlers/decision.rs | 191 +++++++++++++++++++++++++++++++-- 5 files changed, 216 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6ddeb796..e1555b3c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -473,6 +473,26 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b5320ae4c3782150d900b79807611a59a99fc9a1d61d686faafc24b93fc8d7ca" +[[package]] +name = "factori" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4ff6b50917609e530c145de1c6aa8df9c38c40562375e8aa5eeaaf6c737a0b31" +dependencies = [ + "factori-impl", +] + +[[package]] +name = "factori-impl" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6d6344ded92b0a4a1d90a816632f7ff2a12e01401d325d6295810dacca1dbdd6" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "fake-simd" version = "0.1.2" @@ -2124,6 +2144,7 @@ dependencies = [ "cron", "cynic", "dotenv", + "factori", "futures", "github-graphql", "glob", diff --git a/Cargo.toml b/Cargo.toml index 7cf33897..510f716d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,6 +45,7 @@ ignore = "0.4.18" postgres-types = { version = "0.2.4", features = ["derive"] } cron = { version = "0.12.0" } pretty_assertions = "1.2" +factori = "1.1.0" [dependencies.serde] version = "1" diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 5857a500..7291940a 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -87,6 +87,16 @@ pub enum Resolution { #[postgres(name = "hold")] Hold, } + +impl fmt::Display for Resolution { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Resolution::Merge => write!(f, "merge"), + Resolution::Hold => write!(f, "hold"), + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/db/issue_decision_state.rs b/src/db/issue_decision_state.rs index 5091880b..af673c3d 100644 --- a/src/db/issue_decision_state.rs +++ b/src/db/issue_decision_state.rs @@ -14,7 +14,7 @@ pub struct IssueDecisionState { pub initiator: String, pub start_date: DateTime, pub end_date: DateTime, - pub current: BTreeMap, + pub current: BTreeMap>, pub history: BTreeMap>, pub reversibility: Reversibility, pub resolution: Resolution, @@ -34,7 +34,7 @@ pub async fn insert_issue_decision_state( initiator: &String, start_date: &DateTime, end_date: &DateTime, - current: &BTreeMap, + current: &BTreeMap>, history: &BTreeMap>, reversibility: &Reversibility, resolution: &Resolution, @@ -97,7 +97,8 @@ fn deserialize_issue_decision_state(row: &tokio_postgres::row::Row) -> Result = row.try_get(2)?; let end_date: DateTime = row.try_get(3)?; - let current: BTreeMap = serde_json::from_value(row.try_get(4).unwrap())?; + let current: BTreeMap> = + serde_json::from_value(row.try_get(4).unwrap())?; let history: BTreeMap> = serde_json::from_value(row.try_get(5).unwrap())?; let reversibility: Reversibility = row.try_get(6)?; diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 6cf6c9e5..34143fea 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,8 +1,10 @@ use crate::db::jobs::*; +use crate::github; use crate::{ config::DecisionConfig, db::issue_decision_state::*, github::Event, handlers::Context, interactions::ErrorComment, }; +use anyhow::bail; use anyhow::Context as Ctx; use chrono::{DateTime, Duration, Utc}; use parser::command::decision::Resolution::{Hold, Merge}; @@ -93,16 +95,26 @@ pub(super) async fn handle_command( let end_date: DateTime = start_date.checked_add_signed(Duration::days(10)).unwrap(); - let mut current: BTreeMap = BTreeMap::new(); + //TODO: change this to be configurable in toml / ask user to provide the team name + // it should match the same team that we check for above when determining if the user is a member + let team = github::get_team(&ctx.github, &"T-lang").await?.unwrap(); + + let mut current: BTreeMap> = BTreeMap::new(); + + for member in team.members { + current.insert(member.name, None); + } + current.insert( - "mcass19".to_string(), - UserStatus { + user.login.clone(), + Some(UserStatus { comment_id: "comment_id".to_string(), text: "something".to_string(), reversibility: Reversibility::Reversible, resolution: Merge, - }, + }), ); + let history: BTreeMap> = BTreeMap::new(); insert_issue_decision_state( @@ -120,7 +132,11 @@ pub(super) async fn handle_command( let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { message: "some message".to_string(), - get_issue_url: format!("{}/issues/{}", issue.repository().url(), issue.number), + get_issue_url: format!( + "{}/issues/{}", + issue.repository().url(), + issue.number + ), status: Merge, }) .unwrap(); @@ -133,12 +149,7 @@ pub(super) async fn handle_command( ) .await?; - // let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? - - let comment = format!( - "Wow, it looks like you want to merge this, {}.\n| Team member | State |\n|-------------|-------|\n| julmontesdeoca | merge |\n| mcass19 | |", - user.login - ); + let comment = build_status_comment(&history, ¤t)?; issue .post_comment(&ctx.github, &comment) @@ -152,6 +163,164 @@ pub(super) async fn handle_command( } } +fn build_status_comment( + history: &BTreeMap>, + current: &BTreeMap>, +) -> anyhow::Result { + let mut comment = "| Team member | State |\n|-------------|-------|".to_owned(); + for (user, statuses) in history { + let mut user_statuses = format!("\n| {} |", user); + + // previous stasuses + for status in statuses { + let status_item = format!(" ~~{}~~ ", status.resolution); + user_statuses.push_str(&status_item); + } + + // current status + let user_resolution = match current.get(user) { + Some(current_status) => { + if let Some(status) = current_status { + format!("**{}**", status.resolution) + } else { + "".to_string() + } + } + None => bail!("user {} not present in current statuses list", user), + }; + + let status_item = format!(" {} |", user_resolution); + user_statuses.push_str(&status_item); + + comment.push_str(&user_statuses); + } + + Ok(comment) +} + +#[cfg(test)] +mod tests { + use super::*; + use factori::{create, factori}; + + factori!(UserStatus, { + default { + comment_id = "the-comment-id".to_string(), + text = "this is my argument for making this decision".to_string(), + reversibility = Reversibility::Reversible, + resolution = Resolution::Merge + } + + mixin hold { + resolution = Resolution::Hold + } + }); + + #[test] + fn test_successfuly_build_comment() { + let mut history: BTreeMap> = BTreeMap::new(); + let mut current_statuses: BTreeMap> = BTreeMap::new(); + + // user 1 + let mut user_1_statuses: Vec = Vec::new(); + user_1_statuses.push(create!(UserStatus)); + user_1_statuses.push(create!(UserStatus, :hold)); + + history.insert("Niklaus".to_string(), user_1_statuses); + + current_statuses.insert("Niklaus".to_string(), Some(create!(UserStatus))); + + // user 2 + let mut user_2_statuses: Vec = Vec::new(); + user_2_statuses.push(create!(UserStatus, :hold)); + user_2_statuses.push(create!(UserStatus)); + + history.insert("Barbara".to_string(), user_2_statuses); + + current_statuses.insert("Barbara".to_string(), Some(create!(UserStatus))); + + let build_result = build_status_comment(&history, ¤t_statuses) + .expect("it shouldn't fail building the message"); + let expected_comment = "| Team member | State |\n\ + |-------------|-------|\n\ + | Barbara | ~~hold~~ ~~merge~~ **merge** |\n\ + | Niklaus | ~~merge~~ ~~hold~~ **merge** |" + .to_string(); + + assert_eq!(build_result, expected_comment); + } + + #[test] + fn test_successfuly_build_comment_user_no_votes() { + let mut history: BTreeMap> = BTreeMap::new(); + let mut current_statuses: BTreeMap> = BTreeMap::new(); + + // user 1 + let mut user_1_statuses: Vec = Vec::new(); + user_1_statuses.push(create!(UserStatus)); + user_1_statuses.push(create!(UserStatus, :hold)); + + history.insert("Niklaus".to_string(), user_1_statuses); + + current_statuses.insert("Niklaus".to_string(), Some(create!(UserStatus))); + + // user 2 + let mut user_2_statuses: Vec = Vec::new(); + user_2_statuses.push(create!(UserStatus, :hold)); + user_2_statuses.push(create!(UserStatus)); + + history.insert("Barbara".to_string(), user_2_statuses); + + current_statuses.insert("Barbara".to_string(), Some(create!(UserStatus))); + + // user 3 + history.insert("Tom".to_string(), Vec::new()); + + current_statuses.insert("Tom".to_string(), None); + + let build_result = build_status_comment(&history, ¤t_statuses) + .expect("it shouldn't fail building the message"); + let expected_comment = "| Team member | State |\n\ + |-------------|-------|\n\ + | Barbara | ~~hold~~ ~~merge~~ **merge** |\n\ + | Niklaus | ~~merge~~ ~~hold~~ **merge** |\n\ + | Tom | |" + .to_string(); + + assert_eq!(build_result, expected_comment); + } + + #[test] + fn test_build_comment_inconsistent_users() { + let mut history: BTreeMap> = BTreeMap::new(); + let mut current_statuses: BTreeMap> = BTreeMap::new(); + + // user 1 + let mut user_1_statuses: Vec = Vec::new(); + user_1_statuses.push(create!(UserStatus)); + user_1_statuses.push(create!(UserStatus, :hold)); + + history.insert("Niklaus".to_string(), user_1_statuses); + + current_statuses.insert("Niklaus".to_string(), Some(create!(UserStatus))); + + // user 2 + let mut user_2_statuses: Vec = Vec::new(); + user_2_statuses.push(create!(UserStatus, :hold)); + user_2_statuses.push(create!(UserStatus)); + + history.insert("Barbara".to_string(), user_2_statuses); + + current_statuses.insert("Martin".to_string(), Some(create!(UserStatus))); + + let build_result = build_status_comment(&history, ¤t_statuses); + assert_eq!( + format!("{}", build_result.unwrap_err()), + "user Barbara not present in current statuses list" + ); + } +} + // #[cfg(test)] // mod tests { // use chrono::{Duration, Utc}; From c6fbbd3735a166b1e23c96d2aed39d7b4549cd9d Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Thu, 5 Jan 2023 11:31:22 -0300 Subject: [PATCH 13/15] Support hold as first command and post comment when no first vote --- parser/src/command/decision.rs | 28 +-- src/github.rs | 4 +- src/handlers/decision.rs | 310 +++++++-------------------------- src/handlers/jobs.rs | 34 ++-- 4 files changed, 102 insertions(+), 274 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 7291940a..25eb3684 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -28,24 +28,32 @@ impl DecisionCommand { let mut toks = input.clone(); match toks.peek_token()? { - Some(Token::Word("merge")) => { - command_or_error(input, &mut toks, Self { + Some(Token::Word("merge")) => command_or_error( + input, + &mut toks, + Self { resolution: Resolution::Merge, reversibility: Reversibility::Reversible, - }) - } - Some(Token::Word("hold")) => { - command_or_error(input, &mut toks, Self { + }, + ), + Some(Token::Word("hold")) => command_or_error( + input, + &mut toks, + Self { resolution: Resolution::Hold, reversibility: Reversibility::Reversible, - }) - } + }, + ), _ => Ok(None), } } } -fn command_or_error<'a>(input: &mut Tokenizer<'a>, toks: &mut Tokenizer<'a>, command: DecisionCommand) -> Result, Error<'a>> { +fn command_or_error<'a>( + input: &mut Tokenizer<'a>, + toks: &mut Tokenizer<'a>, + command: DecisionCommand, +) -> Result, Error<'a>> { toks.next_token()?; if let Some(Token::Dot) | Some(Token::EndOfLine) = toks.peek_token()? { *input = toks.clone(); @@ -70,7 +78,7 @@ impl fmt::Display for ParseError { } } -#[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, Copy, ToSql, FromSql, Eq, PartialEq)] #[postgres(name = "reversibility")] pub enum Reversibility { #[postgres(name = "reversible")] diff --git a/src/github.rs b/src/github.rs index 40cc1305..cdb587db 100644 --- a/src/github.rs +++ b/src/github.rs @@ -786,13 +786,13 @@ impl Issue { #[derive(serde::Serialize)] struct MergeIssue<'a> { commit_title: &'a str, - merge_method: &'a str + merge_method: &'a str, } client ._send_req(client.put(&merge_url).json(&MergeIssue { commit_title: "Merged by the bot!", - merge_method: "merge" + merge_method: "merge", })) .await .context("failed to merge issue")?; diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 34143fea..0f83af04 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,4 +1,3 @@ -use crate::db::jobs::*; use crate::github; use crate::{ config::DecisionConfig, db::issue_decision_state::*, github::Event, handlers::Context, @@ -7,23 +6,11 @@ use crate::{ use anyhow::bail; use anyhow::Context as Ctx; use chrono::{DateTime, Duration, Utc}; -use parser::command::decision::Resolution::{Hold, Merge}; +use parser::command::decision::Resolution; use parser::command::decision::*; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; -// get state for issue_id from db - // if no state (first call) - // initialize state - // schedule job if necessary - // send comment to github - // save state - // if state - // apply logic to decide what to do - // schedule job if necessary - // send comment to github - // save state - pub const DECISION_PROCESS_JOB_NAME: &str = "decision_process_action"; #[derive(Serialize, Deserialize)] @@ -62,103 +49,76 @@ pub(super) async fn handle_command( match get_issue_decision_state(&db, &issue.number).await { Ok(_state) => { - // let name = match disposition { - // Hold => "hold".into(), - // Custom(name) => name, - // }; - - // let mut current_statuses = state.current_statuses; - // let mut status_history = state.status_history; - - // if let Some(entry) = current_statuses.get_mut(&user) { - // let past = status_history.entry(user).or_insert(Vec::new()); + // TO DO + let cmnt = ErrorComment::new( + &issue, + "We don't support having more than one vote yet. Coming soon :)", + ); + cmnt.post(&ctx.github).await?; - // past.push(entry.clone()); - - // *entry = UserStatus::new(name, issue_id, comment_id); - // } else { - // current_statuses.insert(user, UserStatus::new("hold".into(), issue_id, comment_id)); - // } - - // Ok(State { - // current_statuses, - // status_history, - // ..state - // }) Ok(()) } _ => { - match resolution { - Hold => Ok(()), // change me! - Merge => { - let start_date: DateTime = chrono::Utc::now().into(); - let end_date: DateTime = - start_date.checked_add_signed(Duration::days(10)).unwrap(); - - //TODO: change this to be configurable in toml / ask user to provide the team name - // it should match the same team that we check for above when determining if the user is a member - let team = github::get_team(&ctx.github, &"T-lang").await?.unwrap(); - - let mut current: BTreeMap> = BTreeMap::new(); - - for member in team.members { - current.insert(member.name, None); - } - - current.insert( - user.login.clone(), - Some(UserStatus { - comment_id: "comment_id".to_string(), - text: "something".to_string(), - reversibility: Reversibility::Reversible, - resolution: Merge, - }), - ); - - let history: BTreeMap> = BTreeMap::new(); - - insert_issue_decision_state( - &db, - &issue.number, - &user.login, - &start_date, - &end_date, - ¤t, - &history, - &reversibility, - &Merge, - ) - .await?; - - let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { - message: "some message".to_string(), - get_issue_url: format!( - "{}/issues/{}", - issue.repository().url(), - issue.number - ), - status: Merge, - }) - .unwrap(); - - insert_job( - &db, - &DECISION_PROCESS_JOB_NAME.to_string(), - &end_date, - &metadata, - ) - .await?; - - let comment = build_status_comment(&history, ¤t)?; - - issue - .post_comment(&ctx.github, &comment) - .await - .context("merge vote comment")?; - - Ok(()) - } + let start_date: DateTime = chrono::Utc::now().into(); + let end_date: DateTime = + start_date.checked_add_signed(Duration::days(10)).unwrap(); + + let mut current: BTreeMap> = BTreeMap::new(); + let history: BTreeMap> = BTreeMap::new(); + + // TODO + // change this to be entered by the user as part of the command + // it should match the same team that we check for above when determining if the user is a member + let team = github::get_team(&ctx.github, &"T-lang").await?.unwrap(); + for member in team.members { + current.insert(member.name, None); } + + current.insert( + user.login.clone(), + Some(UserStatus { + comment_id: "comment_id".to_string(), + text: "something".to_string(), + reversibility: reversibility, + resolution: resolution, + }), + ); + + insert_issue_decision_state( + &db, + &issue.number, + &user.login, + &start_date, + &end_date, + ¤t, + &history, + &reversibility, + &resolution, + ) + .await?; + + // TO DO -- Do not insert this job until we support more votes + // let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { + // message: "some message".to_string(), + // get_issue_url: format!("{}/issues/{}", issue.repository().url(), issue.number), + // status: resolution, + // }) + // .unwrap(); + // insert_job( + // &db, + // &DECISION_PROCESS_JOB_NAME.to_string(), + // &end_date, + // &metadata, + // ) + // .await?; + + let comment = build_status_comment(&history, ¤t)?; + issue + .post_comment(&ctx.github, &comment) + .await + .context("merge vote comment")?; + + Ok(()) } } } @@ -320,141 +280,3 @@ mod tests { ); } } - -// #[cfg(test)] -// mod tests { -// use chrono::{Duration, Utc}; -// use pretty_assertions::assert_eq; - -// use super::*; - -// struct TestRenderer {} - -// impl LinkRenderer for TestRenderer { -// fn render_link(&self, data: &UserStatus) -> String { -// let issue_id = &data.issue_id; -// let comment_id = &data.comment_id; - -// format!("http://example.com/issue/{issue_id}#comment={comment_id}") -// } -// } - -// /// Example 1 -// /// -// /// https://lang-team.rust-lang.org/decision_process/examples.html#reversible-decision-merging-a-proposal -// /// -// /// * From the starting point of there not being any state, someone proposes -// /// to merge a proposal -// /// * then barbara holds -// /// * 11 days pass -// /// * barbara says merge, it immediatly merges -// #[test] -// fn example_merging_proposal() { -// let team_members = vec![ -// "@Alan".to_owned(), -// "@Barbara".to_owned(), -// "@Grace".to_owned(), -// "@Niklaus".to_owned(), -// ]; -// let r = TestRenderer {}; - -// // alan proposes to merge -// let time1 = Utc::now(); -// let command = DecisionCommand::merge("@Alan".into(), "1".into(), "1".into()); -// let state = handle_command(None, command, Context::new(team_members.clone(), time1)).unwrap(); - -// assert_eq!(state.period_start, time1); -// assert_eq!(state.original_period_start, time1); -// assert_eq!( -// state.current_statuses, -// vec![( -// "@Alan".into(), -// UserStatus::new("merge".into(), "1".into(), "1".into()) -// ),] -// .into_iter() -// .collect() -// ); -// assert_eq!(state.status_history, HashMap::new()); -// assert_eq!(state.reversibility, Reversibility::Reversible); -// assert_eq!(state.resolution, Custom("merge".into())); -// assert_eq!( -// state.render(&r), -// include_str!("../../test/decision/res/01_merging_proposal__1.md") -// ); - -// // barbara holds -// let time2 = Utc::now(); -// let command = DecisionCommand::hold("@Barbara".into(), "1".into(), "2".into()); -// let state = handle_command( -// Some(state), -// command, -// Context::new(team_members.clone(), time2), -// ) -// .unwrap(); - -// assert_eq!(state.period_start, time1); -// assert_eq!(state.original_period_start, time1); -// assert_eq!( -// state.current_statuses, -// vec![ -// ( -// "@Alan".into(), -// UserStatus::new("merge".into(), "1".into(), "1".into()) -// ), -// ( -// "@Barbara".into(), -// UserStatus::new("hold".into(), "1".into(), "2".into()) -// ), -// ] -// .into_iter() -// .collect() -// ); -// assert_eq!(state.status_history, HashMap::new()); -// assert_eq!(state.reversibility, Reversibility::Reversible); -// assert_eq!(state.resolution, Custom("merge".into())); -// assert_eq!( -// state.render(&r), -// include_str!("../../test/decision/res/01_merging_proposal__2.md") -// ); - -// // 11 days pass -// let time3 = time2 + Duration::days(11); - -// // Barbara says merge, it immediatly merges -// let command = DecisionCommand::merge("@Barbara".into(), "1".into(), "3".into()); -// let state = handle_command(Some(state), command, Context::new(team_members, time3)).unwrap(); - -// assert_eq!(state.period_start, time1); -// assert_eq!(state.original_period_start, time1); -// assert_eq!( -// state.current_statuses, -// vec![ -// ( -// "@Alan".into(), -// UserStatus::new("merge".into(), "1".into(), "1".into()) -// ), -// ( -// "@Barbara".into(), -// UserStatus::new("merge".into(), "1".into(), "3".into()) -// ), -// ] -// .into_iter() -// .collect() -// ); -// assert_eq!( -// state.status_history, -// vec![( -// "@Barbara".into(), -// vec![UserStatus::new("hold".into(), "1".into(), "2".into())] -// ),] -// .into_iter() -// .collect() -// ); -// assert_eq!(state.reversibility, Reversibility::Reversible); -// assert_eq!(state.resolution, Custom("merge".into())); -// assert_eq!( -// state.render(&r), -// include_str!("../../test/decision/01_merging_proposal__3.md") -// ); -// } -// } diff --git a/src/handlers/jobs.rs b/src/handlers/jobs.rs index fa000d49..8c3ac3c2 100644 --- a/src/handlers/jobs.rs +++ b/src/handlers/jobs.rs @@ -4,12 +4,12 @@ // Further info could be find in src/jobs.rs +use super::Context; use crate::github::*; use crate::handlers::decision::{DecisionProcessActionMetadata, DECISION_PROCESS_JOB_NAME}; -use parser::command::decision::Resolution::Merge; +use parser::command::decision::Resolution::{Hold, Merge}; use reqwest::Client; use tracing as log; -use super::Context; pub async fn handle_job( ctx: &Context, @@ -21,10 +21,8 @@ pub async fn handle_job( "rustc_commits" => { super::rustc_commits::synchronize_commits_inner(ctx, None).await; Ok(()) - }, - DECISION_PROCESS_JOB_NAME => { - decision_process_handler(&metadata).await - }, + } + DECISION_PROCESS_JOB_NAME => decision_process_handler(&metadata).await, _ => default(&name, &metadata), } } @@ -46,19 +44,19 @@ async fn decision_process_handler(metadata: &serde_json::Value) -> anyhow::Resul ); let metadata: DecisionProcessActionMetadata = serde_json::from_value(metadata.clone())?; + let gh_client = GithubClient::new_with_default_token(Client::new().clone()); + let request = gh_client.get(&metadata.get_issue_url); - match metadata.status { - Merge => { - let gh_client = GithubClient::new_with_default_token(Client::new().clone()); - - let request = gh_client.get(&metadata.get_issue_url); - - match gh_client.json::(request).await { - Ok(issue) => issue.merge(&gh_client).await?, - Err(e) => log::error!("Failed to get issue {}, error: {}", metadata.get_issue_url, e), - } - } - _ => {} + match gh_client.json::(request).await { + Ok(issue) => match metadata.status { + Merge => issue.merge(&gh_client).await?, + Hold => issue.close(&gh_client).await?, + }, + Err(e) => log::error!( + "Failed to get issue {}, error: {}", + metadata.get_issue_url, + e + ), } Ok(()) From 841caa7317c38a6b6d484558c85aef7902b7f9f0 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 11 Jan 2023 10:59:40 -0300 Subject: [PATCH 14/15] Fix status comment builder --- src/handlers/decision.rs | 65 ++++++++++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 16 deletions(-) diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 0f83af04..b22bb21a 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -64,14 +64,15 @@ pub(super) async fn handle_command( start_date.checked_add_signed(Duration::days(10)).unwrap(); let mut current: BTreeMap> = BTreeMap::new(); - let history: BTreeMap> = BTreeMap::new(); + let mut history: BTreeMap> = BTreeMap::new(); // TODO // change this to be entered by the user as part of the command // it should match the same team that we check for above when determining if the user is a member let team = github::get_team(&ctx.github, &"T-lang").await?.unwrap(); for member in team.members { - current.insert(member.name, None); + current.insert(member.name.clone(), None); + history.insert(member.name.clone(), Vec::new()); } current.insert( @@ -128,25 +129,24 @@ fn build_status_comment( current: &BTreeMap>, ) -> anyhow::Result { let mut comment = "| Team member | State |\n|-------------|-------|".to_owned(); - for (user, statuses) in history { + for (user, status) in current { let mut user_statuses = format!("\n| {} |", user); // previous stasuses - for status in statuses { - let status_item = format!(" ~~{}~~ ", status.resolution); - user_statuses.push_str(&status_item); + match history.get(user) { + Some(statuses) => { + for status in statuses { + let status_item = format!(" ~~{}~~ ", status.resolution); + user_statuses.push_str(&status_item); + } + } + None => bail!("user {} not present in history statuses list", user), } // current status - let user_resolution = match current.get(user) { - Some(current_status) => { - if let Some(status) = current_status { - format!("**{}**", status.resolution) - } else { - "".to_string() - } - } - None => bail!("user {} not present in current statuses list", user), + let user_resolution = match status { + Some(status) => format!("**{}**", status.resolution), + _ => "".to_string(), }; let status_item = format!(" {} |", user_resolution); @@ -276,7 +276,40 @@ mod tests { let build_result = build_status_comment(&history, ¤t_statuses); assert_eq!( format!("{}", build_result.unwrap_err()), - "user Barbara not present in current statuses list" + "user Martin not present in history statuses list" ); } + + #[test] + fn test_successfuly_build_comment_no_history() { + let mut history: BTreeMap> = BTreeMap::new(); + let mut current_statuses: BTreeMap> = BTreeMap::new(); + + // user 1 + let mut user_1_statuses: Vec = Vec::new(); + user_1_statuses.push(create!(UserStatus)); + user_1_statuses.push(create!(UserStatus, :hold)); + + current_statuses.insert("Niklaus".to_string(), Some(create!(UserStatus))); + history.insert("Niklaus".to_string(), Vec::new()); + + // user 2 + let mut user_2_statuses: Vec = Vec::new(); + user_2_statuses.push(create!(UserStatus, :hold)); + user_2_statuses.push(create!(UserStatus)); + + current_statuses.insert("Barbara".to_string(), Some(create!(UserStatus))); + history.insert("Barbara".to_string(), Vec::new()); + + let build_result = build_status_comment(&history, ¤t_statuses) + .expect("it shouldn't fail building the message"); + let expected_comment = "| Team member | State |\n\ + |-------------|-------|\n\ + | Barbara | **merge** |\n\ + | Niklaus | **merge** |\ + " + .to_string(); + + assert_eq!(build_result, expected_comment); + } } From e66216fc3e83d43e1173f9c9fee8f6d76e509c67 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Thu, 19 Jan 2023 15:35:53 -0300 Subject: [PATCH 15/15] Parse and handle team in command * Parse team in command * Use parsed team in handler --- parser/src/command/decision.rs | 94 ++++++++++++++++----- src/handlers/decision.rs | 145 +++++++++++++++++++-------------- 2 files changed, 158 insertions(+), 81 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 25eb3684..289c78e4 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -21,6 +21,7 @@ use serde::{Deserialize, Serialize}; pub struct DecisionCommand { pub resolution: Resolution, pub reversibility: Reversibility, + pub team: Option, } impl DecisionCommand { @@ -28,33 +29,57 @@ impl DecisionCommand { let mut toks = input.clone(); match toks.peek_token()? { - Some(Token::Word("merge")) => command_or_error( - input, - &mut toks, - Self { - resolution: Resolution::Merge, - reversibility: Reversibility::Reversible, - }, - ), - Some(Token::Word("hold")) => command_or_error( - input, - &mut toks, - Self { - resolution: Resolution::Hold, - reversibility: Reversibility::Reversible, - }, - ), + Some(Token::Word("merge")) => { + toks.next_token()?; + + let team: Option = get_team(&mut toks)?; + + command_or_error( + input, + &mut toks, + Self { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible, + team, + }, + ) + } + Some(Token::Word("hold")) => { + toks.next_token()?; + + let team: Option = get_team(&mut toks)?; + + command_or_error( + input, + &mut toks, + Self { + resolution: Resolution::Hold, + reversibility: Reversibility::Reversible, + team, + }, + ) + } _ => Ok(None), } } } +fn get_team<'a>(toks: &mut Tokenizer<'a>) -> Result, Error<'a>> { + match toks.peek_token()? { + Some(Token::Word(team)) => { + toks.next_token()?; + + Ok(Some(team.to_string())) + } + _ => Ok(None), + } +} + fn command_or_error<'a>( input: &mut Tokenizer<'a>, toks: &mut Tokenizer<'a>, command: DecisionCommand, ) -> Result, Error<'a>> { - toks.next_token()?; if let Some(Token::Dot) | Some(Token::EndOfLine) = toks.peek_token()? { *input = toks.clone(); Ok(Some(command)) @@ -120,7 +145,8 @@ mod tests { parse("merge"), Ok(Some(DecisionCommand { resolution: Resolution::Merge, - reversibility: Reversibility::Reversible + reversibility: Reversibility::Reversible, + team: None })), ); } @@ -131,7 +157,8 @@ mod tests { parse("merge."), Ok(Some(DecisionCommand { resolution: Resolution::Merge, - reversibility: Reversibility::Reversible + reversibility: Reversibility::Reversible, + team: None })), ); } @@ -142,7 +169,8 @@ mod tests { parse("hold"), Ok(Some(DecisionCommand { resolution: Resolution::Hold, - reversibility: Reversibility::Reversible + reversibility: Reversibility::Reversible, + team: None })), ); } @@ -151,7 +179,7 @@ mod tests { fn test_expected_end() { use std::error::Error; assert_eq!( - parse("hold my beer") + parse("hold lang beer") .unwrap_err() .source() .unwrap() @@ -159,4 +187,28 @@ mod tests { Some(&ParseError::ExpectedEnd), ); } + + #[test] + fn test_correct_merge_with_team() { + assert_eq!( + parse("merge lang"), + Ok(Some(DecisionCommand { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible, + team: Some("lang".to_string()) + })), + ); + } + + #[test] + fn test_correct_hold_with_team() { + assert_eq!( + parse("hold lang"), + Ok(Some(DecisionCommand { + resolution: Resolution::Hold, + reversibility: Reversibility::Reversible, + team: Some("lang".to_string()) + })), + ); + } } diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index b22bb21a..8818cdc2 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -31,6 +31,7 @@ pub(super) async fn handle_command( let DecisionCommand { resolution, reversibility, + team: team_name, } = cmd; let issue = event.issue().unwrap(); @@ -59,67 +60,91 @@ pub(super) async fn handle_command( Ok(()) } _ => { - let start_date: DateTime = chrono::Utc::now().into(); - let end_date: DateTime = - start_date.checked_add_signed(Duration::days(10)).unwrap(); - - let mut current: BTreeMap> = BTreeMap::new(); - let mut history: BTreeMap> = BTreeMap::new(); - - // TODO - // change this to be entered by the user as part of the command - // it should match the same team that we check for above when determining if the user is a member - let team = github::get_team(&ctx.github, &"T-lang").await?.unwrap(); - for member in team.members { - current.insert(member.name.clone(), None); - history.insert(member.name.clone(), Vec::new()); + match team_name { + None => { + let cmnt = ErrorComment::new( + &issue, + "In the first vote, is necessary to specify the team name that will be involved in the decision process.", + ); + cmnt.post(&ctx.github).await?; + + Ok(()) + } + Some(team_name) => { + match github::get_team(&ctx.github, &team_name).await { + Ok(Some(team)) => { + let start_date: DateTime = chrono::Utc::now().into(); + let end_date: DateTime = + start_date.checked_add_signed(Duration::days(10)).unwrap(); + + let mut current: BTreeMap> = BTreeMap::new(); + let mut history: BTreeMap> = BTreeMap::new(); + + // Add team members to current and history + for member in team.members { + current.insert(member.github.clone(), None); + history.insert(member.github.clone(), Vec::new()); + } + + // Add issue user to current and history + current.insert( + user.login.clone(), + Some(UserStatus { + comment_id: event.html_url().unwrap().to_string(), + text: event.comment_body().unwrap().to_string(), + reversibility: reversibility, + resolution: resolution, + }), + ); + history.insert(user.login.clone(), Vec::new()); + + // Initialize issue decision state + insert_issue_decision_state( + &db, + &issue.number, + &user.login, + &start_date, + &end_date, + ¤t, + &history, + &reversibility, + &resolution, + ) + .await?; + + // TO DO -- Do not insert this job until we support more votes + // let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { + // message: "some message".to_string(), + // get_issue_url: format!("{}/issues/{}", issue.repository().url(), issue.number), + // status: resolution, + // }) + // .unwrap(); + // insert_job( + // &db, + // &DECISION_PROCESS_JOB_NAME.to_string(), + // &end_date, + // &metadata, + // ) + // .await?; + + let comment = build_status_comment(&history, ¤t)?; + issue + .post_comment(&ctx.github, &comment) + .await + .context("merge vote comment")?; + + Ok(()) + } + _ => { + let cmnt = + ErrorComment::new(&issue, "Failed to resolve to a known team."); + cmnt.post(&ctx.github).await?; + + Ok(()) + } + } + } } - - current.insert( - user.login.clone(), - Some(UserStatus { - comment_id: "comment_id".to_string(), - text: "something".to_string(), - reversibility: reversibility, - resolution: resolution, - }), - ); - - insert_issue_decision_state( - &db, - &issue.number, - &user.login, - &start_date, - &end_date, - ¤t, - &history, - &reversibility, - &resolution, - ) - .await?; - - // TO DO -- Do not insert this job until we support more votes - // let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { - // message: "some message".to_string(), - // get_issue_url: format!("{}/issues/{}", issue.repository().url(), issue.number), - // status: resolution, - // }) - // .unwrap(); - // insert_job( - // &db, - // &DECISION_PROCESS_JOB_NAME.to_string(), - // &end_date, - // &metadata, - // ) - // .await?; - - let comment = build_status_comment(&history, ¤t)?; - issue - .post_comment(&ctx.github, &comment) - .await - .context("merge vote comment")?; - - Ok(()) } } }