From debb1abcd7e8378567fbf0b7b6c18082ab9cc14a Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Tue, 17 Dec 2024 00:24:54 +0100 Subject: [PATCH] feat(checkouts): generalize change fetching via git references This way, we can plug any VCS and just leave it to the Git reference converter. We implement a very simple one for Gerrit. Signed-off-by: Raito Bezarius --- ofborg/src/bin/build-faker.rs | 2 ++ ofborg/src/checkout.rs | 22 ++++++++++++++++++---- ofborg/src/message/common.rs | 22 ++++++++++++++++++++++ ofborg/src/tasks/build.rs | 4 ++-- ofborg/src/tasks/evaluate.rs | 4 ++-- ofborg/src/tasks/gerrit/mod.rs | 13 ++++++++++--- ofborg/src/vcs/gerrit/data_structures.rs | 8 ++++++-- 7 files changed, 62 insertions(+), 13 deletions(-) diff --git a/ofborg/src/bin/build-faker.rs b/ofborg/src/bin/build-faker.rs index 7d999a1..84cf229 100644 --- a/ofborg/src/bin/build-faker.rs +++ b/ofborg/src/bin/build-faker.rs @@ -29,6 +29,8 @@ async fn main() -> Result<(), Box> { }; let pr_msg = Change { + provenance: ofborg::tasks::evaluate::SupportedVCS::Gerrit, + r#ref: "refs/pull/42/head:pr".to_string(), number: 42, head_sha: "6dd9f0265d52b946dd13daf996f30b64e4edb446".to_owned(), target_branch: Some("scratch".to_owned()), diff --git a/ofborg/src/checkout.rs b/ofborg/src/checkout.rs index 25cc37b..5cdfff2 100644 --- a/ofborg/src/checkout.rs +++ b/ofborg/src/checkout.rs @@ -1,6 +1,7 @@ use crate::clone::{self, GitClonable}; use std::ffi::{OsStr, OsString}; +use std::fmt::Debug; use std::fs; use std::io::{Error, ErrorKind}; use std::path::{Path, PathBuf}; @@ -31,6 +32,11 @@ pub struct CachedProjectCo { local_reference: PathBuf, } +pub trait ToGitRef { + /// This transforms the current change representation to a git reference that you can fetch. + fn to_git_ref(&self) -> String; +} + impl CachedCloner { #[must_use] pub fn project(&self, name: &str, clone_url: String) -> CachedProject { @@ -96,14 +102,14 @@ impl CachedProjectCo { Ok(self.clone_to().to_str().unwrap().to_string()) } - pub fn fetch_pr(&self, pr_id: u64) -> Result<(), Error> { + pub fn fetch_ref(&self, git_ref: &str) -> Result<(), Error> { let mut lock = self.lock()?; - info!("Fetching PR #{}", pr_id); + info!("Fetching ref #{}", git_ref); let result = Command::new("git") .arg("fetch") .arg("origin") - .arg(format!("+refs/pull/{pr_id}/head:pr")) + .arg(format!("+{git_ref}")) .current_dir(self.clone_to()) .stdout(Stdio::null()) .status()?; @@ -113,10 +119,18 @@ impl CachedProjectCo { if result.success() { Ok(()) } else { - Err(Error::new(ErrorKind::Other, "Failed to fetch PR")) + Err(Error::new( + ErrorKind::Other, + format!("Failed to fetch reference {git_ref}"), + )) } } + pub fn fetch_change(&self, change: &C) -> Result<(), Error> { + tracing::debug!("Fetching change {:?}", change); + self.fetch_ref(&change.to_git_ref()) + } + pub fn commit_exists(&self, commit: &OsStr) -> bool { let mut lock = self.lock().expect("Failed to lock"); diff --git a/ofborg/src/message/common.rs b/ofborg/src/message/common.rs index d62ca85..19eb89b 100644 --- a/ofborg/src/message/common.rs +++ b/ofborg/src/message/common.rs @@ -1,3 +1,7 @@ +use std::fmt::Display; + +use crate::{checkout::ToGitRef, tasks::evaluate::SupportedVCS}; + #[derive(Serialize, Deserialize, Debug, Clone)] pub struct Repo { pub owner: String, @@ -8,7 +12,25 @@ pub struct Repo { #[derive(Serialize, Deserialize, Debug, Clone)] pub struct Change { + pub provenance: SupportedVCS, pub target_branch: Option, pub number: u64, pub head_sha: String, + // Git reference for this change + pub r#ref: String, +} + +impl ToGitRef for Change { + fn to_git_ref(&self) -> String { + self.r#ref.clone() + } +} + +impl Display for Change { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_fmt(format_args!( + "VCS change (prov: {:?}) #{} targeting {:?} on {}", + self.provenance, self.number, self.target_branch, self.r#ref + )) + } } diff --git a/ofborg/src/tasks/build.rs b/ofborg/src/tasks/build.rs index 31db951..cf486f1 100644 --- a/ofborg/src/tasks/build.rs +++ b/ofborg/src/tasks/build.rs @@ -335,8 +335,8 @@ impl notifyworker::SimpleNotifyWorker for BuildWorker { let refpath = co.checkout_origin_ref(target_branch.as_ref()).unwrap(); - if co.fetch_pr(job.change.number).is_err() { - info!("Failed to fetch {}", job.change.number); + if co.fetch_change(&job.change).is_err() { + info!("Failed to fetch {}", job.change); actions.pr_head_missing().await; return; } diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index 3cd3fe7..404d3cf 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -21,6 +21,7 @@ use std::time::Instant; use async_trait::async_trait; use tracing::{debug_span, error, info, warn}; +#[derive(Debug, Clone, Deserialize, Serialize)] pub enum SupportedVCS { Gerrit, } @@ -373,8 +374,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { .set_with_description("Fetching PR", State::Pending) .await?; - // TODO: generalize fetch change - co.fetch_pr(job.change.number).unwrap(); + co.fetch_change(&job.change).unwrap(); if !co.commit_exists(job.change.head_sha.as_ref()) { overall_status diff --git a/ofborg/src/tasks/gerrit/mod.rs b/ofborg/src/tasks/gerrit/mod.rs index 0b1be41..38ede61 100644 --- a/ofborg/src/tasks/gerrit/mod.rs +++ b/ofborg/src/tasks/gerrit/mod.rs @@ -1,5 +1,6 @@ /// This transforms a Gerrit event into a `OfBorg` event. use crate::message::vcs::VCSEvent; +use crate::tasks::evaluate::SupportedVCS; use crate::vcs::gerrit::data_structures::GerritStreamEvent; use crate::worker; use async_trait::async_trait; @@ -60,11 +61,13 @@ impl worker::SimpleWorker for GerritEventFilterWorker { GerritStreamEvent::ChangeDeleted { change, .. } | GerritStreamEvent::ChangeAbandoned { change, .. } => Some(VCSEvent::ChangeDeleted { change: crate::message::Change { + provenance: SupportedVCS::Gerrit, target_branch: Some(change.branch.clone()), number: change .change_number .expect("Change deletion payload should contain a CL number"), - head_sha: change.current_patch_set.as_ref().unwrap().r#ref.clone(), + head_sha: change.current_patch_set.as_ref().unwrap().revision.clone(), + r#ref: change.current_patch_set.as_ref().unwrap().r#ref.clone(), }, repo: crate::message::Repo { owner: "no one really".to_string(), @@ -76,6 +79,7 @@ impl worker::SimpleWorker for GerritEventFilterWorker { GerritStreamEvent::ChangeRestored { change, .. } | GerritStreamEvent::PatchSetCreated { change, .. } => Some(VCSEvent::ChangeCreated { change: crate::message::Change { + provenance: SupportedVCS::Gerrit, target_branch: Some(change.branch.clone()), number: change .change_number @@ -84,8 +88,9 @@ impl worker::SimpleWorker for GerritEventFilterWorker { .current_patch_set .as_ref() .expect("Patch set creation payload should contain a current patch set") - .r#ref + .revision .clone(), + r#ref: change.current_patch_set.as_ref().unwrap().r#ref.clone(), }, repo: crate::message::Repo { owner: "no one really".to_string(), @@ -98,9 +103,11 @@ impl worker::SimpleWorker for GerritEventFilterWorker { change, patch_set, .. } => Some(VCSEvent::WIPStatusChange { change: crate::message::Change { + provenance: SupportedVCS::Gerrit, target_branch: Some(change.branch.clone()), number: change.change_number.unwrap(), - head_sha: patch_set.r#ref.clone(), + head_sha: patch_set.revision.clone(), + r#ref: patch_set.r#ref.clone(), }, wip: change.wip.unwrap(), }), diff --git a/ofborg/src/vcs/gerrit/data_structures.rs b/ofborg/src/vcs/gerrit/data_structures.rs index a9a538d..c1142b3 100644 --- a/ofborg/src/vcs/gerrit/data_structures.rs +++ b/ofborg/src/vcs/gerrit/data_structures.rs @@ -2,6 +2,8 @@ use std::collections::HashMap; use serde::{Deserialize, Serialize}; +use crate::tasks::evaluate::SupportedVCS; + #[derive(Serialize, Deserialize, Debug)] pub struct Account { pub name: Option, // User's full name, if configured @@ -212,8 +214,9 @@ pub struct Change { } impl From for crate::message::Change { - fn from(value: Change) -> Self { + fn from(mut value: Change) -> Self { Self { + provenance: SupportedVCS::Gerrit, target_branch: Some(value.branch), // While the change number is deprecated, we actually need it. // FIXME: enforce type level checking of this. @@ -222,7 +225,8 @@ impl From for crate::message::Change { // basically, some events like `comment-added` does not include the current patch set // because it's already included in another field, we could probably do another // conversion pass to improve the type safety. - head_sha: value.current_patch_set.unwrap().revision, + head_sha: std::mem::take(&mut value.current_patch_set.as_mut().unwrap().revision), + r#ref: std::mem::take(&mut value.current_patch_set.as_mut().unwrap().r#ref), } } }