Skip to content

Commit

Permalink
refactor to use dedicated handler for config validation
Browse files Browse the repository at this point in the history
  • Loading branch information
meysam81 committed Oct 14, 2023
1 parent f31459a commit 84017db
Show file tree
Hide file tree
Showing 3 changed files with 137 additions and 36 deletions.
20 changes: 15 additions & 5 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::sync::{Arc, RwLock};
use std::time::{Duration, Instant};
use tracing as log;

static CONFIG_FILE_NAME: &str = "triagebot.toml";
pub(crate) static CONFIG_FILE_NAME: &str = "triagebot.toml";
const REFRESH_EVERY: Duration = Duration::from_secs(2 * 60); // Every two minutes

lazy_static::lazy_static! {
Expand All @@ -15,10 +15,6 @@ lazy_static::lazy_static! {
RwLock::new(HashMap::new());
}

pub fn config_file_name() -> &'static str {
CONFIG_FILE_NAME
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
#[serde(rename_all = "kebab-case")]
pub(crate) struct Config {
Expand All @@ -38,6 +34,9 @@ pub(crate) struct Config {
pub(crate) note: Option<NoteConfig>,
pub(crate) mentions: Option<MentionsConfig>,
pub(crate) no_merges: Option<NoMergesConfig>,
// We want this validation to run even without the entry in the config file
#[serde(default = "ValidateConfig::default")]
pub(crate) validate_config: Option<ValidateConfig>,
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
Expand Down Expand Up @@ -160,6 +159,16 @@ pub(crate) struct PrioritizeConfig {
pub(crate) label: String,
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
pub(crate) struct ValidateConfig {
}

impl ValidateConfig {
fn default() -> Option<Self> {
Some(ValidateConfig { })
}
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
pub(crate) struct AutolabelConfig {
#[serde(flatten)]
Expand Down Expand Up @@ -427,6 +436,7 @@ mod tests {
review_submitted: None,
mentions: None,
no_merges: None,
validate_config: None,
}
);
}
Expand Down
33 changes: 2 additions & 31 deletions src/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ mod review_submitted;
mod rfc_helper;
pub mod rustc_commits;
mod shortcut;
mod validate_config;

pub async fn handle(ctx: &Context, event: &Event) -> Vec<HandlerError> {
let config = config::get(&ctx.github, event.repo()).await;
Expand Down Expand Up @@ -124,29 +125,6 @@ pub async fn handle(ctx: &Context, event: &Event) -> Vec<HandlerError> {
errors
}

async fn validate_triagebot(ctx: &Context, event: &IssuesEvent, errors: &mut Vec<HandlerError>) {
if let Some(pr_source) = &event.issue.head {
if let Some(Some(triagebot_toml)) = ctx
.github
.raw_file(
&pr_source.repo.full_name,
&pr_source.git_ref,
crate::config::config_file_name(),
)
.await
.ok()
{
if let Err(err) = toml::from_slice::<crate::handlers::Config>(&triagebot_toml) {
errors.push(HandlerError::Message(format!(
"The triagebot.toml file is not a valid config file.\n\
Error: {}",
err
)));
}
}
}
}

macro_rules! issue_handlers {
($($name:ident,)*) => {
async fn handle_issue(
Expand All @@ -155,14 +133,6 @@ macro_rules! issue_handlers {
config: &Arc<Config>,
errors: &mut Vec<HandlerError>,
) {
if event.issue.is_pr() {
if let Ok(Some(diff)) = event.issue.diff(&ctx.github).await {
if diff.contains(config::config_file_name()) {
validate_triagebot(&ctx, &event, errors).await;
}
}
}

$(
match $name::parse_input(ctx, event, config.$name.as_ref()).await {
Err(err) => errors.push(HandlerError::Message(err)),
Expand Down Expand Up @@ -195,6 +165,7 @@ issue_handlers! {
mentions,
no_merges,
notify_zulip,
validate_config,
}

macro_rules! command_handlers {
Expand Down
120 changes: 120 additions & 0 deletions src/handlers/validate_config.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
//! For pull requests that have changed the triagebot.toml, validate that the
//! changes are a valid configuration file.
//! It won't validate anything unless the PR is open and has changed.
//! For every event, it will fetch the diff of the pull request which is one
//! round-trip of HTTP request to the GitHub API, and another round-trip to
//! fetch the triagebot.toml if it has changed.

use crate::{
config::ValidateConfig,
github::{IssuesAction},
handlers::{Context, IssuesEvent},
};
use tracing as log;

/// If the issue is a pull request and if among the changed files, triagebot.toml
/// is changed, then validate the triagebot.toml config file with the syntax
/// parser from the config module.
pub(super) async fn parse_input(
ctx: &Context,
event: &IssuesEvent,
_config: Option<&ValidateConfig>,
) -> Result<Option<()>, String> {
if !event.issue.is_pr() {
log::debug!("Ignoring issue: {:?}", event.issue);
return Ok(None);
}

if !matches!(
event.action,
IssuesAction::Opened | IssuesAction::Synchronize | IssuesAction::ReadyForReview,
) {
log::debug!("Ignoring issue action: {:?}", event.action);
return Ok(None);
}

let diff = match event.issue.diff(&ctx.github).await {
Ok(Some(diff)) => diff,
wildcard => {
log::error!("Failed to fetch diff: {:?}", wildcard);
return Ok(None);
}
};

if diff.contains(crate::config::CONFIG_FILE_NAME) {
log::debug!("Validating triagebot config");
match validate_triagebot_config(ctx, event).await {
Ok(()) => {
log::debug!("Valid triagebot config");
return Ok(Some(()));
}
Err(e) => {
log::debug!("Invalid triagebot config: {:?}", e);
return Err(e);
}
}
}

Ok(None)
}

async fn validate_triagebot_config(ctx: &Context, event: &IssuesEvent) -> Result<(), String> {
if let Some(pr_source) = &event.issue.head {
if let Ok(Some(triagebot_toml)) = ctx
.github
.raw_file(
&pr_source.repo.full_name,
&pr_source.git_ref,
crate::config::CONFIG_FILE_NAME,
)
.await
{
match toml::from_slice::<crate::handlers::Config>(&triagebot_toml) {
Ok(_) => return Ok(()),
Err(e) => {
let position = e.line_col().map(|(line, col)| format!("{}:{}", line + 1, col + 1));
let triagebot_toml_blob_url = event
.issue
.files(&ctx.github)
.await
.expect("Failed to fetch files")
.iter()
.find(|file| file.filename == crate::config::CONFIG_FILE_NAME)
.unwrap()
.blob_url
.clone();

let absolute_url = format!(
"{}#L{}",
triagebot_toml_blob_url,
position.clone().unwrap_or_default()
);

return Err(format!(
"Invalid triagebot.toml at position {}",
format!(
"[{}]({})",
position.unwrap_or_default(),
absolute_url
)
));
}
}
}
}

Ok(())
}

pub(super) async fn handle_input(
_ctx: &Context,
_config: &ValidateConfig,
_event: &IssuesEvent,
_input: (),
) -> anyhow::Result<()> {
Ok(())
}

pub(super) struct ValidationInput {
diff: String,
}

0 comments on commit 84017db

Please sign in to comment.