diff --git a/Cargo.lock b/Cargo.lock index eecbb71..62d5e0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1041,6 +1041,7 @@ dependencies = [ "anyhow", "buildomat-common", "buildomat-github-database", + "buildomat-jobsh", "serde", "serde_json", "toml", diff --git a/Cargo.toml b/Cargo.toml index 0a77a5f..c723433 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,6 +26,10 @@ members = [ ] resolver = "2" +[workspace.lints.clippy] +too_many_arguments = "allow" +wrong_self_convention = "allow" + [workspace.dependencies] ansi-to-html = { version = "0.2", features = [ "lazy-init" ] } anyhow = "1" diff --git a/agent/src/control/mod.rs b/agent/src/control/mod.rs index b26493a..21ebe6a 100644 --- a/agent/src/control/mod.rs +++ b/agent/src/control/mod.rs @@ -18,7 +18,15 @@ use protocol::{Decoder, FactoryInfo, Message, Payload}; pub(crate) mod protocol; pub(crate) mod server; -pub const SOCKET_PATH: &str = "/var/run/buildomat.sock"; +/// Default socket path. +const DEFAULT_SOCKET_PATH: &str = "/var/run/buildomat.sock"; + +/// Get the control socket path for client (bmat) connections. +/// Uses BUILDOMAT_SOCKET env var if set (set by agent when running jobs), +/// otherwise falls back to the default path. +fn socket_path() -> String { + std::env::var("BUILDOMAT_SOCKET").unwrap_or_else(|_| DEFAULT_SOCKET_PATH.to_string()) +} struct Stuff { us: Option, @@ -28,7 +36,7 @@ struct Stuff { impl Stuff { async fn connect(&mut self) -> Result<()> { - self.us = Some(UnixStream::connect(SOCKET_PATH).await?); + self.us = Some(UnixStream::connect(socket_path()).await?); self.dec = Some(Decoder::new()); Ok(()) } @@ -178,13 +186,13 @@ async fn cmd_address_list(mut l: Level) -> Result<()> { let mut r = Row::default(); r.add_str("name", &addr.name); - r.add_str("cidr", &net.to_string()); - r.add_str("first", &first.to_string()); - r.add_str("last", &last.to_string()); + r.add_str("cidr", net.to_string()); + r.add_str("first", first.to_string()); + r.add_str("last", last.to_string()); r.add_u64("count", addr.count.into()); r.add_str("family", "inet"); - r.add_str("network", &net.network().to_string()); - r.add_str("mask", &net.netmask().to_string()); + r.add_str("network", net.network().to_string()); + r.add_str("mask", net.netmask().to_string()); r.add_str("routed", if addr.routed { "yes" } else { "no" }); r.add_str("gateway", addr.gateway.as_deref().unwrap_or("-")); @@ -578,3 +586,21 @@ async fn cmd_factory_private(mut l: Level) -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_socket_path_default() { + std::env::remove_var("BUILDOMAT_SOCKET"); + assert_eq!(socket_path(), "/var/run/buildomat.sock"); + } + + #[test] + fn test_socket_path_from_env() { + std::env::set_var("BUILDOMAT_SOCKET", "/custom/path/buildomat.sock"); + assert_eq!(socket_path(), "/custom/path/buildomat.sock"); + std::env::remove_var("BUILDOMAT_SOCKET"); + } +} diff --git a/agent/src/control/server.rs b/agent/src/control/server.rs index 29bfe9a..69e78b9 100644 --- a/agent/src/control/server.rs +++ b/agent/src/control/server.rs @@ -15,10 +15,7 @@ use tokio::{ }, }; -use super::{ - protocol::{Decoder, Message, Payload}, - SOCKET_PATH, -}; +use super::protocol::{Decoder, Message, Payload}; #[derive(Debug)] pub struct Request { @@ -45,20 +42,25 @@ impl Request { } } -pub fn listen() -> Result> { +pub fn listen(socket_path: &std::path::Path) -> Result> { /* * Create the UNIX socket that the control program will use to contact the * agent. */ - std::fs::remove_file(SOCKET_PATH).ok(); - let ul = UnixListener::bind(SOCKET_PATH)?; + // Create parent directory if it doesn't exist (needed for persistent mode) + if let Some(parent) = socket_path.parent() { + std::fs::create_dir_all(parent).ok(); + } + + std::fs::remove_file(socket_path).ok(); + let ul = UnixListener::bind(socket_path)?; /* * Allow everyone to connect: */ - let mut perm = std::fs::metadata(SOCKET_PATH)?.permissions(); + let mut perm = std::fs::metadata(socket_path)?.permissions(); perm.set_mode(0o777); - std::fs::set_permissions(SOCKET_PATH, perm)?; + std::fs::set_permissions(socket_path, perm)?; /* * Create channel to hand requests back to the main loop. diff --git a/agent/src/exec.rs b/agent/src/exec.rs index 6a82677..e7c8184 100644 --- a/agent/src/exec.rs +++ b/agent/src/exec.rs @@ -23,10 +23,7 @@ fn spawn_reader( where T: Read + Send + 'static, { - let stream = match stream { - Some(stream) => stream, - None => return None, - }; + let stream = stream?; Some(std::thread::spawn(move || { let mut r = BufReader::new(stream); @@ -297,7 +294,7 @@ fn run_common( * process. */ if ab.bgproc.is_none() { - tx.blocking_send(ab.exit(&start, &end, std::i32::MAX)) + tx.blocking_send(ab.exit(&start, &end, i32::MAX)) .unwrap(); } @@ -332,7 +329,7 @@ fn run_common( let code = if let Some(code) = es.code() { code } else { - std::i32::MAX + i32::MAX }; tx.blocking_send(ab.exit(&start, &end, code)).unwrap(); stdio_warning diff --git a/agent/src/main.rs b/agent/src/main.rs index 4705440..6334bee 100644 --- a/agent/src/main.rs +++ b/agent/src/main.rs @@ -42,17 +42,17 @@ use exec::ExitDetails; struct Agent { log: Logger, + /// Path to the config file. Set via -c flag. + config_path: PathBuf, } -const CONFIG_PATH: &str = "/opt/buildomat/etc/agent.json"; -const JOB_PATH: &str = "/opt/buildomat/etc/job.json"; -const AGENT: &str = "/opt/buildomat/lib/agent"; const INPUT_PATH: &str = "/input"; const CONTROL_PROGRAM: &str = "bmat"; const SHADOW: &str = "/etc/shadow"; +/// Default config file path. +const DEFAULT_CONFIG_PATH: &str = "/opt/buildomat/etc/agent.json"; #[cfg(target_os = "illumos")] mod os_constants { - pub const METHOD: &str = "/opt/buildomat/lib/start.sh"; pub const MANIFEST: &str = "/var/svc/manifest/site/buildomat-agent.xml"; pub const INPUT_DATASET: &str = "rpool/input"; } @@ -64,14 +64,182 @@ use os_constants::*; use crate::control::protocol::StoreEntry; +fn default_version() -> u32 { + 1 +} + #[derive(Serialize, Deserialize)] struct ConfigFile { + /// Config file version. Missing field defaults to 1 for backward + /// compatibility. + #[serde(default = "default_version")] + version: u32, baseurl: String, bootstrap: String, token: String, + /// Base directory for agent files. Defaults to /opt/buildomat. + #[serde(default)] + opt_base: Option, + /// Persistent mode - skip SMF/systemd/ZFS setup, run as current user. + #[serde(default)] + persistent: bool, } impl ConfigFile { + /// Config file version. + #[allow(dead_code)] + pub fn version(&self) -> u32 { + self.version + } + + /// Base URL for buildomat server. + pub fn baseurl(&self) -> &str { + &self.baseurl + } + + /// Bootstrap token for initial registration. + pub fn bootstrap(&self) -> &str { + &self.bootstrap + } + + /// Authentication token. + pub fn token(&self) -> &str { + &self.token + } + + /// Whether running in persistent mode. + pub fn persistent(&self) -> bool { + self.persistent + } + + /// Base directory for buildomat agent files. + fn opt_base(&self) -> PathBuf { + self.opt_base + .as_ref() + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from("/opt/buildomat")) + } + + /// Path to the job state file (indicates job in progress). + pub fn job_path(&self) -> PathBuf { + self.opt_base().join("etc/job.json") + } + + /// Path to the job completion marker file. + pub fn job_done_path(&self) -> PathBuf { + self.opt_base().join("etc/job-done.json") + } + + /// Path to the agent binary. + pub fn agent_path(&self) -> PathBuf { + self.opt_base().join("lib/agent") + } + + /// Path to the control program (bmat). + /// In persistent mode, installed under opt_base. + /// In normal mode, installed to /usr/bin. + pub fn control_program_path(&self) -> PathBuf { + if self.persistent { + self.opt_base().join("bin").join(CONTROL_PROGRAM) + } else { + PathBuf::from(format!("/usr/bin/{CONTROL_PROGRAM}")) + } + } + + /// Path to the control socket. + /// In persistent mode, under opt_base/run. + /// In normal mode, /var/run/buildomat.sock. + pub fn socket_path(&self) -> PathBuf { + if self.persistent { + self.opt_base().join("run/buildomat.sock") + } else { + PathBuf::from("/var/run/buildomat.sock") + } + } + + /// Path to the SMF method script (illumos only). + #[cfg(target_os = "illumos")] + pub fn method_path(&self) -> PathBuf { + self.opt_base().join("lib/start.sh") + } + + /// Compute the HOME environment variable for task execution. + /// In persistent mode, uses current user's HOME; otherwise /root. + pub fn task_home(&self) -> String { + if self.persistent { + env::var("HOME").unwrap_or_else(|_| "/tmp".to_string()) + } else { + "/root".to_string() + } + } + + /// Compute the USER/LOGNAME environment variable for task execution. + /// In persistent mode, uses current user; otherwise root. + pub fn task_user(&self) -> String { + if self.persistent { + env::var("USER").unwrap_or_else(|_| "nobody".to_string()) + } else { + "root".to_string() + } + } + + /// Compute the working directory for task execution. + /// In persistent mode, uses user's home directory (writable). + /// In normal mode, uses "/" (ephemeral VM, doesn't matter). + pub fn task_cwd(&self) -> PathBuf { + if self.persistent { + PathBuf::from(self.task_home()) + } else { + PathBuf::from("/") + } + } + + /// Compute the PATH environment variable for task execution. + /// In persistent mode, prepends opt_base/bin so bmat is findable. + pub fn task_path(&self) -> String { + let mut paths: Vec = Vec::new(); + + if self.persistent { + paths.push(self.opt_base().join("bin").to_string_lossy().to_string()); + } + + paths.extend([ + "/usr/bin".to_string(), + "/bin".to_string(), + "/usr/sbin".to_string(), + "/sbin".to_string(), + "/opt/ooce/bin".to_string(), + "/opt/ooce/sbin".to_string(), + ]); + + paths.join(":") + } + + /// Mark job as complete by renaming job.json to job-done.json. + /// This allows subsequent agent runs to detect that the previous job + /// completed successfully rather than crashed. + pub fn mark_job_done(&self, log: &Logger) { + let job_file = self.job_path(); + let done_file = self.job_done_path(); + + if job_file.exists() { + match std::fs::rename(&job_file, &done_file) { + Ok(()) => { + info!(log, "marked job as done"; + "from" => %job_file.display(), + "to" => %done_file.display()); + } + Err(e) => { + // Non-fatal - the job still completed + error!(log, "failed to mark job done"; + "from" => %job_file.display(), + "to" => %done_file.display(), + "error" => %e); + } + } + } + } + fn make_client(&self, log: Logger) -> ClientWrap { let client = buildomat_client::ClientBuilder::new(&self.baseurl) .bearer_token(&self.token) @@ -90,7 +258,19 @@ impl ConfigFile { rx, )); - ClientWrap { log, client, job: None, tx: None, worker_tx } + ClientWrap { + log, + client, + job: None, + tx: None, + worker_tx, + persistent: self.persistent, + socket_path: self.socket_path().to_string_lossy().to_string(), + task_path: self.task_path(), + task_home: self.task_home(), + task_user: self.task_user(), + task_cwd: self.task_cwd(), + } } } @@ -284,6 +464,18 @@ pub(crate) struct ClientWrap { job: Option>, tx: Option>, worker_tx: mpsc::Sender, + /// Whether running in persistent mode. + persistent: bool, + /// Path to the socket (for setting BUILDOMAT_SOCKET env var). + socket_path: String, + /// PATH environment variable for task execution. + task_path: String, + /// HOME environment variable for task execution. + task_home: String, + /// USER/LOGNAME environment variable for task execution. + task_user: String, + /// Working directory for task execution. + task_cwd: PathBuf, } impl ClientWrap { @@ -642,23 +834,23 @@ impl ClientWrap { */ cmd.env_clear(); - cmd.env("HOME", "/root"); - cmd.env("USER", "root"); - cmd.env("LOGNAME", "root"); + cmd.env("HOME", &self.task_home); + cmd.env("USER", &self.task_user); + cmd.env("LOGNAME", &self.task_user); cmd.env("TZ", "UTC"); - cmd.env( - "PATH", - "/usr/bin:/bin:/usr/sbin:/sbin:/opt/ooce/bin:/opt/ooce/sbin", - ); + cmd.env("PATH", &self.task_path); cmd.env("LANG", "en_US.UTF-8"); cmd.env("LC_ALL", "en_US.UTF-8"); + cmd.env("BUILDOMAT_SOCKET", &self.socket_path); /* - * Run the diagnostic script as root: + * Run the diagnostic script as root (skip uid/gid in persistent mode): */ - cmd.current_dir("/"); - cmd.uid(0); - cmd.gid(0); + cmd.current_dir(&self.task_cwd); + if !self.persistent { + cmd.uid(0); + cmd.gid(0); + } match exec::run_diagnostic(cmd, name) { Ok(c) => Some(c), @@ -1025,9 +1217,12 @@ enum Stage { async fn cmd_install(mut l: Level) -> Result<()> { l.usage_args(Some("BASEURL BOOTSTRAP_TOKEN")); l.optopt("N", "", "set nodename of machine", "NODENAME"); + l.optopt("", "opt-base", "base directory for agent files", "PATH"); + l.optflag("", "persistent", "persistent mode (skip SMF/systemd/ZFS setup)"); let a = args!(l); let log = l.context().log.clone(); + let config_path = l.context().config_path.clone(); if a.args().len() < 2 { bad_args!(l, "specify base URL and bootstrap token value"); @@ -1039,6 +1234,8 @@ async fn cmd_install(mut l: Level) -> Result<()> { */ let baseurl = a.args()[0].to_string(); let bootstrap = a.args()[1].to_string(); + let persistent = a.opts().opt_present("persistent"); + let opt_base = a.opts().opt_str("opt-base"); if let Some(nodename) = a.opts().opt_str("N") { if let Err(e) = set_nodename(&nodename) { @@ -1047,41 +1244,53 @@ async fn cmd_install(mut l: Level) -> Result<()> { } /* - * Write /opt/buildomat/etc/agent.json with this configuration. + * Write the agent configuration file. */ - make_dirs_for(CONFIG_PATH)?; - rmfile(CONFIG_PATH)?; - let cf = ConfigFile { baseurl, bootstrap, token: genkey(64) }; - store(CONFIG_PATH, &cf)?; + make_dirs_for(&config_path)?; + rmfile(&config_path)?; + let cf = ConfigFile { + version: 2, + baseurl, + bootstrap, + token: genkey(64), + opt_base, + persistent, + }; + store(&config_path, &cf)?; /* * Copy the agent binary into a permanent home. */ let exe = env::current_exe()?; - make_dirs_for(AGENT)?; - rmfile(AGENT)?; - std::fs::copy(&exe, AGENT)?; - make_executable(AGENT)?; + let agent_path = cf.agent_path(); + make_dirs_for(&agent_path)?; + rmfile(&agent_path)?; + std::fs::copy(&exe, &agent_path)?; + make_executable(&agent_path)?; /* * Install the agent binary with the control program name in a location in * the default PATH so that job programs can find it. + * In persistent mode, we install to opt_base/bin/ instead of /usr/bin/. */ - let cprog = format!("/usr/bin/{CONTROL_PROGRAM}"); + let cprog = cf.control_program_path(); + make_dirs_for(&cprog)?; rmfile(&cprog)?; std::fs::copy(&exe, &cprog)?; make_executable(&cprog)?; #[cfg(target_os = "illumos")] - { + if !cf.persistent() { /* * Copy SMF method script and manifest into place. + * Skipped in persistent mode as these require root privileges. */ - let method = include_str!("../smf/start.sh"); - make_dirs_for(METHOD)?; - rmfile(METHOD)?; - write_text(METHOD, method)?; - make_executable(METHOD)?; + let method_content = include_str!("../smf/start.sh"); + let method = cf.method_path(); + make_dirs_for(&method)?; + rmfile(&method)?; + write_text(&method, method_content)?; + make_executable(&method)?; let manifest = include_str!("../smf/agent.xml"); rmfile(MANIFEST)?; @@ -1089,11 +1298,12 @@ async fn cmd_install(mut l: Level) -> Result<()> { /* * Create the input directory. + * Skipped in persistent mode - input dir is provided externally. */ let status = Command::new("/sbin/zfs") .arg("create") .arg("-o") - .arg(&format!("mountpoint={}", INPUT_PATH)) + .arg(format!("mountpoint={INPUT_PATH}")) .arg(INPUT_DATASET) .env_clear() .current_dir("/") @@ -1106,6 +1316,7 @@ async fn cmd_install(mut l: Level) -> Result<()> { /* * Import SMF service. + * Skipped in persistent mode - agent is run directly by the factory. */ let status = Command::new("/usr/sbin/svccfg") .arg("import") @@ -1121,14 +1332,16 @@ async fn cmd_install(mut l: Level) -> Result<()> { } #[cfg(target_os = "linux")] - { + if !cf.persistent() { /* * Create the input directory. + * Skipped in persistent mode - input dir is provided externally. */ std::fs::create_dir_all(INPUT_PATH)?; /* * Write a systemd unit file for the agent service. + * Skipped in persistent mode - agent is run directly by the factory. */ let unit = include_str!("../systemd/agent.service"); make_dirs_for(UNIT)?; @@ -1182,17 +1395,18 @@ async fn cmd_install(mut l: Level) -> Result<()> { async fn cmd_run(mut l: Level) -> Result<()> { no_args!(l); - let cf = load::<_, ConfigFile>(CONFIG_PATH)?; + let config_path = l.context().config_path.clone(); + let cf = load::<_, ConfigFile>(&config_path)?; let log = l.context().log.clone(); - info!(log, "agent starting"; "baseurl" => &cf.baseurl); + info!(log, "agent starting"; "baseurl" => cf.baseurl()); let mut cw = cf.make_client(log.clone()); let res = loop { let res = cw .client .worker_bootstrap() - .body_map(|body| body.bootstrap(&cf.bootstrap).token(&cf.token)) + .body_map(|body| body.bootstrap(cf.bootstrap()).token(cf.token())) .send() .await; @@ -1215,7 +1429,7 @@ async fn cmd_run(mut l: Level) -> Result<()> { let mut pingfreq = tokio::time::interval(Duration::from_secs(5)); pingfreq.set_missed_tick_behavior(MissedTickBehavior::Skip); - let mut control = control::server::listen()?; + let mut control = control::server::listen(&cf.socket_path())?; let mut creq: Option = None; let mut bgprocs = exec::BackgroundProcesses::new(); @@ -1236,7 +1450,7 @@ async fn cmd_run(mut l: Level) -> Result<()> { * any evidence that we've done this before, we must report it to the * central server and do nothing else. */ - if PathBuf::from(JOB_PATH).try_exists()? { + if cf.job_path().try_exists()? { error!(log, "found previously assigned job; reporting failure"); /* @@ -1374,7 +1588,7 @@ async fn cmd_run(mut l: Level) -> Result<()> { .create_new(true) .create(false) .write(true) - .open(JOB_PATH)?; + .open(cf.job_path())?; jf.write_all(&diag)?; jf.flush()?; jf.sync_all()?; @@ -1599,21 +1813,17 @@ async fn cmd_run(mut l: Level) -> Result<()> { /* * Absent a request for an entirely clean slate, we * set a few specific environment variables. - * XXX HOME/USER/LOGNAME should probably respect "uid" */ - cmd.env("HOME", "/root"); - cmd.env("USER", "root"); - cmd.env("LOGNAME", "root"); + cmd.env("HOME", &cw.task_home); + cmd.env("USER", &cw.task_user); + cmd.env("LOGNAME", &cw.task_user); cmd.env("TZ", "UTC"); - cmd.env( - "PATH", - "/usr/bin:/bin:/usr/sbin:/sbin:\ - /opt/ooce/bin:/opt/ooce/sbin", - ); + cmd.env("PATH", &cw.task_path); cmd.env("LANG", "en_US.UTF-8"); cmd.env("LC_ALL", "en_US.UTF-8"); cmd.env("BUILDOMAT_JOB_ID", cw.job_id().unwrap()); cmd.env("BUILDOMAT_TASK_ID", t.id.to_string()); + cmd.env("BUILDOMAT_SOCKET", &cw.socket_path); } for (k, v) in t.env.iter() { /* @@ -1627,10 +1837,16 @@ async fn cmd_run(mut l: Level) -> Result<()> { /* * Each task may be expected to run under a different user * account or with a different working directory. + * + * In persistent mode, we skip the uid/gid setting and run as + * the current user. This allows testing on persistent testbeds + * without requiring root privileges. */ cmd.current_dir(&t.workdir); - cmd.uid(t.uid); - cmd.gid(t.gid); + if !cw.persistent { + cmd.uid(t.uid); + cmd.gid(t.gid); + } match exec::run(cmd) { Ok(c) => { @@ -1808,9 +2024,11 @@ async fn cmd_run(mut l: Level) -> Result<()> { stage = Stage::PostDiagnostics(c, None); } else { cw.diagnostics_complete(false).await; + cf.mark_job_done(&log); stage = Stage::Complete; } } else { + cf.mark_job_done(&log); stage = Stage::Complete; continue; } @@ -1846,6 +2064,7 @@ async fn cmd_run(mut l: Level) -> Result<()> { */ cw.diagnostics_complete(failed.unwrap()).await; + cf.mark_job_done(&log); stage = Stage::Complete; } None => { @@ -1853,6 +2072,7 @@ async fn cmd_run(mut l: Level) -> Result<()> { .await; cw.diagnostics_complete(false).await; + cf.mark_job_done(&log); stage = Stage::Complete; } } @@ -1861,6 +2081,32 @@ async fn cmd_run(mut l: Level) -> Result<()> { } } +/// Parse the -c CONFIG_PATH option from command line arguments. +/// Returns the config path and the remaining arguments with -c removed. +fn parse_config_path_arg() -> (PathBuf, Vec) { + let args: Vec = std::env::args().collect(); + let mut config_path = PathBuf::from(DEFAULT_CONFIG_PATH); + let mut filtered_args = Vec::new(); + let mut skip_next = false; + + for (i, arg) in args.iter().enumerate() { + if skip_next { + skip_next = false; + continue; + } + if arg == "-c" { + if let Some(path) = args.get(i + 1) { + config_path = PathBuf::from(path); + skip_next = true; + } + } else { + filtered_args.push(arg.clone()); + } + } + + (config_path, filtered_args) +} + #[tokio::main] async fn main() -> Result<()> { let cmdname = std::env::args().next().as_deref().and_then(|s| { @@ -1881,10 +2127,17 @@ async fn main() -> Result<()> { /* * Assume that any name other than the name for the control * entrypoint is the regular agent. + * + * Parse the -c option before hiercmd takes over. + * This allows: agent -c /path/to/config.json install ... */ + let (config_path, _filtered_args) = parse_config_path_arg(); + let log = make_log("buildomat-agent"); - let mut l = - Level::new("buildomat-agent", Agent { log: log.clone() }); + let mut l = Level::new( + "buildomat-agent", + Agent { log: log.clone(), config_path }, + ); l.cmd("install", "install the agent", cmd!(cmd_install))?; l.cmd("run", "run the agent", cmd!(cmd_run))?; @@ -1897,3 +2150,230 @@ async fn main() -> Result<()> { } } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Helper to create a test ConfigFile with defaults. + fn test_config() -> ConfigFile { + ConfigFile { + version: 2, + baseurl: "http://test".to_string(), + bootstrap: "bootstrap".to_string(), + token: "token".to_string(), + opt_base: None, + persistent: false, + } + } + + /// Helper to create a test ConfigFile with custom opt_base. + fn test_config_with_opt_base(opt_base: &str) -> ConfigFile { + ConfigFile { + version: 2, + baseurl: "http://test".to_string(), + bootstrap: "bootstrap".to_string(), + token: "token".to_string(), + opt_base: Some(opt_base.to_string()), + persistent: false, + } + } + + /// Helper to create a persistent test ConfigFile. + fn test_config_persistent() -> ConfigFile { + ConfigFile { + version: 2, + baseurl: "http://test".to_string(), + bootstrap: "bootstrap".to_string(), + token: "token".to_string(), + opt_base: None, + persistent: true, + } + } + + #[test] + fn test_job_path_default() { + let cf = test_config(); + assert_eq!(cf.job_path(), PathBuf::from("/opt/buildomat/etc/job.json")); + } + + #[test] + fn test_job_path_custom_opt() { + let cf = test_config_with_opt_base("/custom/path"); + assert_eq!(cf.job_path(), PathBuf::from("/custom/path/etc/job.json")); + } + + #[test] + fn test_agent_path_default() { + let cf = test_config(); + assert_eq!(cf.agent_path(), PathBuf::from("/opt/buildomat/lib/agent")); + } + + #[test] + fn test_persistent_default() { + let cf = test_config(); + assert!(!cf.persistent()); + } + + #[test] + fn test_persistent_enabled() { + let cf = test_config_persistent(); + assert!(cf.persistent()); + } + + #[test] + fn test_job_done_path_default() { + let cf = test_config(); + assert_eq!( + cf.job_done_path(), + PathBuf::from("/opt/buildomat/etc/job-done.json") + ); + } + + #[test] + fn test_control_program_path_normal_mode() { + let cf = test_config(); + assert_eq!(cf.control_program_path(), PathBuf::from("/usr/bin/bmat")); + } + + #[test] + fn test_control_program_path_persistent_mode() { + let cf = test_config_persistent(); + assert_eq!( + cf.control_program_path(), + PathBuf::from("/opt/buildomat/bin/bmat") + ); + } + + #[test] + fn test_control_program_path_persistent_custom_opt() { + let cf = ConfigFile { + version: 2, + baseurl: "http://test".to_string(), + bootstrap: "bootstrap".to_string(), + token: "token".to_string(), + opt_base: Some("/custom/path".to_string()), + persistent: true, + }; + assert_eq!( + cf.control_program_path(), + PathBuf::from("/custom/path/bin/bmat") + ); + } + + #[test] + fn test_socket_path_normal_mode() { + let cf = test_config(); + assert_eq!( + cf.socket_path(), + PathBuf::from("/var/run/buildomat.sock") + ); + } + + #[test] + fn test_socket_path_persistent_mode() { + let cf = test_config_persistent(); + assert_eq!( + cf.socket_path(), + PathBuf::from("/opt/buildomat/run/buildomat.sock") + ); + } + + #[test] + fn test_task_path_normal_mode() { + let cf = test_config(); + assert_eq!( + cf.task_path(), + "/usr/bin:/bin:/usr/sbin:/sbin:/opt/ooce/bin:/opt/ooce/sbin" + ); + } + + #[test] + fn test_task_path_persistent_mode() { + let cf = test_config_persistent(); + assert_eq!( + cf.task_path(), + "/opt/buildomat/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/ooce/bin:/opt/ooce/sbin" + ); + } + + #[test] + fn test_task_home_normal_mode() { + let cf = test_config(); + assert_eq!(cf.task_home(), "/root"); + } + + #[test] + fn test_task_user_normal_mode() { + let cf = test_config(); + assert_eq!(cf.task_user(), "root"); + } + + #[test] + fn test_task_cwd_normal_mode() { + let cf = test_config(); + assert_eq!(cf.task_cwd(), PathBuf::from("/")); + } + + #[test] + fn test_task_cwd_persistent_mode() { + let cf = test_config_persistent(); + // In persistent mode, task_cwd uses task_home which reads env var + // For this test, we just verify it's not "/" + assert_ne!(cf.task_cwd(), PathBuf::from("/")); + } + + #[test] + fn test_task_path_persistent_custom_opt() { + let cf = ConfigFile { + version: 2, + baseurl: "http://test".to_string(), + bootstrap: "bootstrap".to_string(), + token: "token".to_string(), + opt_base: Some("/custom/path".to_string()), + persistent: true, + }; + assert_eq!( + cf.task_path(), + "/custom/path/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/ooce/bin:/opt/ooce/sbin" + ); + } + + #[test] + fn test_version_default_deserialization() { + // Test that missing version field defaults to 1 + let json = r#"{"baseurl":"http://test","bootstrap":"b","token":"t"}"#; + let cf: ConfigFile = serde_json::from_str(json).unwrap(); + assert_eq!(cf.version(), 1); + assert!(!cf.persistent()); + assert!(cf.opt_base.is_none()); + } + + #[test] + fn test_version_2_deserialization() { + let json = r#"{"version":2,"baseurl":"http://test","bootstrap":"b","token":"t","persistent":true,"opt_base":"/custom"}"#; + let cf: ConfigFile = serde_json::from_str(json).unwrap(); + assert_eq!(cf.version(), 2); + assert!(cf.persistent()); + assert_eq!(cf.opt_base.as_deref(), Some("/custom")); + } + + #[cfg(target_os = "illumos")] + mod illumos_tests { + use super::*; + + #[test] + fn test_method_path_default() { + let cf = test_config(); + assert_eq!( + cf.method_path(), + PathBuf::from("/opt/buildomat/lib/start.sh") + ); + } + + #[test] + fn test_manifest_path_value() { + assert_eq!(MANIFEST, "/var/svc/manifest/site/buildomat-agent.xml"); + } + } +} diff --git a/bin/src/main.rs b/bin/src/main.rs index 71b340c..ad7a5d5 100644 --- a/bin/src/main.rs +++ b/bin/src/main.rs @@ -26,11 +26,11 @@ mod config; trait FlagsExt { #[must_use] - fn add_flags(&mut self, name: &'static str) -> Flags; + fn add_flags(&mut self, name: &'static str) -> Flags<'_>; } impl FlagsExt for Row { - fn add_flags(&mut self, name: &'static str) -> Flags { + fn add_flags(&mut self, name: &'static str) -> Flags<'_> { Flags { row: self, name, out: String::new() } } } @@ -1226,7 +1226,7 @@ async fn do_job_store_list(mut l: Level) -> Result<()> { ); r.add_str( "updated", - &ent.time_update.to_rfc3339_opts(chrono::SecondsFormat::Secs, true), + ent.time_update.to_rfc3339_opts(chrono::SecondsFormat::Secs, true), ); t.add_row(r); @@ -1351,7 +1351,7 @@ async fn do_user_list(mut l: Level) -> Result<()> { r.add_str("name", &u.name); r.add_str( "creation", - &u.time_create.to_rfc3339_opts(chrono::SecondsFormat::Secs, true), + u.time_create.to_rfc3339_opts(chrono::SecondsFormat::Secs, true), ); r.add_age("age", u.time_create.age()); t.add_row(r); diff --git a/download/Cargo.toml b/download/Cargo.toml index fba1796..6a0c8a0 100644 --- a/download/Cargo.toml +++ b/download/Cargo.toml @@ -4,6 +4,9 @@ version = "0.0.0" edition = "2021" license = "MPL-2.0" +[lints] +workspace = true + [lib] doctest = false diff --git a/download/src/lib.rs b/download/src/lib.rs index 5a5ec4b..03c3e55 100644 --- a/download/src/lib.rs +++ b/download/src/lib.rs @@ -43,7 +43,7 @@ fn make_get_response( rx: mpsc::Receiver, E>>, ) -> Result> where - E: Into> + E: Into> + Send + Sync + 'static, diff --git a/factory/aws/src/aws.rs b/factory/aws/src/aws.rs index 237d34f..f455dcd 100644 --- a/factory/aws/src/aws.rs +++ b/factory/aws/src/aws.rs @@ -70,7 +70,7 @@ impl Instance { .try_into() .unwrap(); - (if when <= now { now - when } else { 0 }) / 1000 + now.saturating_sub(when) / 1000 }) .unwrap_or(0) } @@ -188,7 +188,7 @@ async fn create_instance( let mut instances = res .instances() - .into_iter() + .iter() .map(|i| Instance::from((i, config.aws.tag.as_str()))) .collect::>(); @@ -218,13 +218,12 @@ async fn instances( Ok(res .reservations() .iter() - .map(|r| { + .flat_map(|r| { r.instances().iter().map(|i| { let i = Instance::from((i, tag)); (i.id.to_string(), i) }) }) - .flatten() .collect()) } @@ -421,7 +420,7 @@ async fn aws_worker_one( * There is a record of a particular instance ID for this worker. * Check to see if that instance exists. */ - if let Some(i) = insts.get(&instance_id.to_string()) { + if let Some(i) = insts.get(instance_id) { if i.state == "terminated" { /* * The instance exists, but is terminated. Delete the diff --git a/factory/aws/src/main.rs b/factory/aws/src/main.rs index 4c7c96c..f3a510d 100644 --- a/factory/aws/src/main.rs +++ b/factory/aws/src/main.rs @@ -72,7 +72,7 @@ impl Central { * Allow the per-target diagnostic configuration to override the base * diagnostic configuration. */ - Ok(self.config.diag.apply_overrides(&t.diag).build()?) + self.config.diag.apply_overrides(&t.diag).build() } } diff --git a/factory/gimlet/Cargo.toml b/factory/gimlet/Cargo.toml index 2d0858d..d4e14ce 100644 --- a/factory/gimlet/Cargo.toml +++ b/factory/gimlet/Cargo.toml @@ -4,6 +4,9 @@ version = "0.0.0" edition = "2021" license = "MPL-2.0" +[lints] +workspace = true + [dependencies] buildomat-common = { path = "../../common" } buildomat-database = { path = "../../database" } diff --git a/factory/gimlet/src/cleanup.rs b/factory/gimlet/src/cleanup.rs index 2919a57..9eccb3c 100644 --- a/factory/gimlet/src/cleanup.rs +++ b/factory/gimlet/src/cleanup.rs @@ -110,7 +110,7 @@ pub fn setup() -> Result<()> { bail!("giving up after {MAX_TIME} seconds"); } - if let Ok(st) = svcs(&fmri) { + if let Ok(st) = svcs(fmri) { if st.next.is_none() && st.current == "ON" { println!(" * {fmri} now online!"); break; @@ -125,7 +125,7 @@ pub fn setup() -> Result<()> { * only be one! */ let pools = zpool_unimported_list()?; - if pools.len() == 0 { + if pools.is_empty() { bail!("no unimported pool found!"); } else if pools.len() > 1 { bail!("more than one unimported pool found!"); @@ -137,14 +137,14 @@ pub fn setup() -> Result<()> { }; println!(" * importing pool {pool:?}..."); - zpool_import(&pool)?; + zpool_import(pool)?; /* * Update the BSU symlink: */ std::fs::create_dir_all("/pool/bsu")?; std::fs::remove_file("/pool/bsu/0").ok(); - std::os::unix::fs::symlink(&format!("../int/{pool_id}"), "/pool/bsu/0")?; + std::os::unix::fs::symlink(format!("../int/{pool_id}"), "/pool/bsu/0")?; /* * Create the swap device: @@ -252,14 +252,14 @@ fn prepare_m2() -> Result<()> { let p = &mut vtoc.parts_mut()[i]; p.p_tag = efi::sys::V_USR; p.p_start = next_start; - p.p_size = size.into(); + p.p_size = size; next_start = next_start.checked_add(size).unwrap(); } /* * Create the dump slice: */ - let size = (1 * 1024 * 1024 * 1024 * 1024) / vtoc.block_size(); + let size = (1024 * 1024 * 1024 * 1024) / vtoc.block_size(); let p = &mut vtoc.parts_mut()[4]; p.p_tag = efi::sys::V_USR; p.p_start = next_start; @@ -287,11 +287,11 @@ fn prepare_m2() -> Result<()> { .env_clear() .arg("create") .arg("-O") - .arg(&format!("mountpoint=/pool/int/{id}")) + .arg(format!("mountpoint=/pool/int/{id}")) .arg("-O") .arg("compress=on") .arg(&pool) - .arg(&format!("{}s5", d.name)) + .arg(format!("{}s5", d.name)) .output()?; if !out.status.success() { diff --git a/factory/gimlet/src/disks.rs b/factory/gimlet/src/disks.rs index c7fe058..d738684 100644 --- a/factory/gimlet/src/disks.rs +++ b/factory/gimlet/src/disks.rs @@ -121,7 +121,7 @@ pub fn list_disks() -> Result { slice: u32| -> Result> { let mut paths = links - .links_for_path(&device)? + .links_for_path(device)? .into_iter() .filter(|l| { l.linktype() == devinfo::DevLinkType::Primary @@ -193,18 +193,18 @@ pub fn list_disks() -> Result { */ let device = m.devfs_path()?; - Ok((0..=8) + (0..=8) .map(|num| { Ok(slice_from_path(&device, num)?.map(|link| { (num, m.spec_type(), link.path().to_owned()) })) }) - .collect::>>()?) + .collect::>>() }) .collect::>>()? .into_iter() .flatten() - .filter_map(|v| v) + .flatten() .collect::>(); /* diff --git a/factory/gimlet/src/host.rs b/factory/gimlet/src/host.rs index 399c987..74cf715 100644 --- a/factory/gimlet/src/host.rs +++ b/factory/gimlet/src/host.rs @@ -624,7 +624,7 @@ impl HostManager { Ok(()) } HostState::Cleaning(cst) => { - let mut new_cst = cst.clone(); + let mut new_cst = *cst; { let mut l = self.locked.lock().unwrap(); @@ -700,7 +700,7 @@ impl HostManager { Ok(()) } HostState::Starting(sst, hgs) => { - let mut new_sst = sst.clone(); + let mut new_sst = *sst; { let mut l = self.locked.lock().unwrap(); @@ -726,7 +726,7 @@ impl HostManager { } } - if let Err(e) = self.thread_starting(&mut new_sst, &hgs) { + if let Err(e) = self.thread_starting(&mut new_sst, hgs) { error!(log, "starting error: {e}"); } @@ -794,7 +794,7 @@ impl HostManager { let pb = sys .ssh_exec("svcs -Ho sta,nsta svc:/site/postboot:default")?; - let t = pb.out.trim().split_whitespace().collect::>(); + let t = pb.out.split_whitespace().collect::>(); if t.len() != 2 || t[0] == "ON" || t[1] == "-" { bail!("postboot not ready: {t:?}"); } @@ -847,7 +847,7 @@ impl HostManager { let pb = sys .ssh_exec("svcs -Ho sta,nsta svc:/site/postboot:default")?; - let t = pb.out.trim().split_whitespace().collect::>(); + let t = pb.out.split_whitespace().collect::>(); if t.len() != 2 || t[0] == "ON" || t[1] == "-" { bail!("postboot not ready: {t:?}"); } diff --git a/factory/gimlet/src/humility.rs b/factory/gimlet/src/humility.rs index ec43267..1611ba8 100644 --- a/factory/gimlet/src/humility.rs +++ b/factory/gimlet/src/humility.rs @@ -57,7 +57,7 @@ impl ValueExt for Value { bail!("expected a hex number, got {t:?}"); }; - out.push(u8::from_str_radix(&hex, 16)?); + out.push(u8::from_str_radix(hex, 16)?); } _ => bail!("list should only contain terms"), } diff --git a/factory/gimlet/src/instance.rs b/factory/gimlet/src/instance.rs index 64a69b1..1580175 100644 --- a/factory/gimlet/src/instance.rs +++ b/factory/gimlet/src/instance.rs @@ -83,7 +83,7 @@ async fn instance_worker_start( } instances_with_workers.insert(id.clone()); - let c = Arc::clone(&c); + let c = Arc::clone(c); let log = log.new(o!( "component" => "instance_worker", "instance" => id.to_string(), diff --git a/factory/lab/src/main.rs b/factory/lab/src/main.rs index 5a7380b..1a31ab4 100644 --- a/factory/lab/src/main.rs +++ b/factory/lab/src/main.rs @@ -2,6 +2,8 @@ * Copyright 2023 Oxide Computer Company */ +#![allow(dead_code)] + use std::{ collections::HashMap, sync::{Arc, Mutex}, diff --git a/factory/lab/src/minder.rs b/factory/lab/src/minder.rs index 5773e2a..98732d0 100644 --- a/factory/lab/src/minder.rs +++ b/factory/lab/src/minder.rs @@ -74,7 +74,7 @@ impl FormatScript { .replace("%BASEURL%", &self.host_config.lab_baseurl) .replace("%HOST%", &self.host_config.nodename) .replace("%CONSOLE%", &self.host_config.console) - .replace("%BOOTARGS%", &bootargs) + .replace("%BOOTARGS%", bootargs) .replace("%COREURL%", &self.coreurl); if let Some(key) = self.key.as_deref() { @@ -149,7 +149,7 @@ impl CentralExt for Central { &self, name: &str, ) -> HResult<&super::config::ConfigFileHost> { - if let Some(h) = self.hosts.get(&name.to_string()) { + if let Some(h) = self.hosts.get(name) { Ok(&h.config) } else { Err(dropshot::HttpError::for_client_error( diff --git a/factory/propolis/src/config.rs b/factory/propolis/src/config.rs index 2bcc1a2..23fbcbf 100644 --- a/factory/propolis/src/config.rs +++ b/factory/propolis/src/config.rs @@ -93,7 +93,7 @@ impl ConfigFile { &self, slot: u32, id: &InstanceId, - ) -> Result { + ) -> Result> { Ok(InstanceInSlot { config: self, slot, id: id.clone() }) } diff --git a/factory/propolis/src/main.rs b/factory/propolis/src/main.rs index 7e0cde2..19873dc 100644 --- a/factory/propolis/src/main.rs +++ b/factory/propolis/src/main.rs @@ -39,7 +39,7 @@ impl Central { * Allow the per-target diagnostic configuration to override the base * diagnostic configuration. */ - Ok(self.config.diag.apply_overrides(&t.diag).build()?) + self.config.diag.apply_overrides(&t.diag).build() } } diff --git a/factory/propolis/src/vm.rs b/factory/propolis/src/vm.rs index 3e25a52..896a51a 100644 --- a/factory/propolis/src/vm.rs +++ b/factory/propolis/src/vm.rs @@ -461,8 +461,8 @@ async fn instance_worker_one( include_str!("../smf/serial.xml"), ), ] { - std::fs::write(&vmdir.join(&format!("{name}.sh")), script)?; - std::fs::write(&smfdir.join(format!("{name}.xml")), bundle)?; + std::fs::write(vmdir.join(format!("{name}.sh")), script)?; + std::fs::write(smfdir.join(format!("{name}.xml")), bundle)?; } std::fs::write(&siteprofile, include_str!("../smf/site.xml"))?; diff --git a/github/database/src/tables/check_run.rs b/github/database/src/tables/check_run.rs index 1244d19..9cb56dd 100644 --- a/github/database/src/tables/check_run.rs +++ b/github/database/src/tables/check_run.rs @@ -216,7 +216,7 @@ impl CheckRunDependency { */ #[cfg(test)] mod test { - use super::CheckRunVariety; + use super::*; use std::str::FromStr; const CHECK_RUN_VARIETIES: &'static [(&'static str, CheckRunVariety)] = &[ @@ -239,4 +239,75 @@ mod test { assert_eq!(CheckRunVariety::from_str(s).unwrap(), *e); } } + + fn make_check_run(dependencies: Option) -> CheckRun { + CheckRun { + id: CheckRunId::generate(), + check_suite: CheckSuiteId::generate(), + name: "test-run".to_string(), + variety: CheckRunVariety::Basic, + content: None, + config: None, + private: None, + active: true, + flushed: false, + github_id: None, + dependencies: dependencies.map(JsonValue), + } + } + + #[test] + fn check_run_get_dependencies_none() { + let cr = make_check_run(None); + let deps = cr.get_dependencies().unwrap(); + assert!(deps.is_empty()); + } + + #[test] + fn check_run_get_dependencies_empty() { + let cr = make_check_run(Some(serde_json::json!({}))); + let deps = cr.get_dependencies().unwrap(); + assert!(deps.is_empty()); + } + + #[test] + fn check_run_get_dependencies_single() { + let cr = make_check_run(Some(serde_json::json!({ + "build": { + "job": "build-job", + "config": {} + } + }))); + let deps = cr.get_dependencies().unwrap(); + assert_eq!(deps.len(), 1); + assert_eq!(deps.get("build").unwrap().job(), "build-job"); + } + + #[test] + fn check_run_get_dependencies_multiple() { + let cr = make_check_run(Some(serde_json::json!({ + "build": { + "job": "build-job", + "config": {"copy_outputs": true} + }, + "test": { + "job": "test-job", + "config": {} + } + }))); + let deps = cr.get_dependencies().unwrap(); + assert_eq!(deps.len(), 2); + assert_eq!(deps.get("build").unwrap().job(), "build-job"); + assert_eq!(deps.get("test").unwrap().job(), "test-job"); + + // Verify we can extract config from dependency + #[derive(serde::Deserialize)] + struct DepConfig { + #[serde(default)] + copy_outputs: bool, + } + let build_config: DepConfig = + deps.get("build").unwrap().get_config().unwrap(); + assert!(build_config.copy_outputs); + } } diff --git a/github/database/src/tables/check_suite.rs b/github/database/src/tables/check_suite.rs index 3fd23d9..c538552 100644 --- a/github/database/src/tables/check_suite.rs +++ b/github/database/src/tables/check_suite.rs @@ -61,7 +61,7 @@ impl From for JobFile { content, dependencies: dependencies .into_iter() - .map(|(a, b)| (a.into(), b.into())) + .map(|(a, b)| (a, b.into())) .collect(), } } diff --git a/github/database/src/tables/user.rs b/github/database/src/tables/user.rs index f4fcf3e..a2379ea 100644 --- a/github/database/src/tables/user.rs +++ b/github/database/src/tables/user.rs @@ -123,4 +123,34 @@ mod test { assert_eq!(UserType::from_str(s).unwrap(), *e); } } + + /* + * Test conversion from GitHub API strings. + */ + const GITHUB_USER_TYPES: &[(&str, UserType)] = &[ + ("User", UserType::User), + ("Bot", UserType::Bot), + ("Organization", UserType::Organisation), + ]; + + #[test] + fn user_type_from_github_str() { + for (s, e) in GITHUB_USER_TYPES { + assert_eq!(UserType::from_github_str(s).unwrap(), *e); + } + } + + #[test] + fn user_type_from_github_str_invalid() { + let invalid_types = &["user", "bot", "org", "Invalid", "", "Org"]; + for invalid in invalid_types { + let err = UserType::from_github_str(invalid).unwrap_err(); + assert!( + err.to_string().contains("invalid user type"), + "expected invalid user type error for {:?}, got: {}", + invalid, + err + ); + } + } } diff --git a/github/dbtool/src/main.rs b/github/dbtool/src/main.rs index a70b21c..52f4cc3 100644 --- a/github/dbtool/src/main.rs +++ b/github/dbtool/src/main.rs @@ -22,11 +22,11 @@ const SHORT_SHA_LEN: usize = 16; trait FlagsExt { #[must_use] - fn add_flags(&mut self, name: &'static str) -> Flags; + fn add_flags(&mut self, name: &'static str) -> Flags<'_>; } impl FlagsExt for Row { - fn add_flags(&mut self, name: &'static str) -> Flags { + fn add_flags(&mut self, name: &'static str) -> Flags<'_> { Flags { row: self, name, out: String::new() } } } @@ -69,7 +69,7 @@ impl Stuff { std::fs::DirBuilder::new().mode(0o700).recursive(true).create(&out)?; out.push(set); std::fs::DirBuilder::new().mode(0o700).recursive(true).create(&out)?; - out.push(&format!("{}.json", file)); + out.push(format!("{file}.json")); Ok(out) } } @@ -290,7 +290,7 @@ async fn do_delivery_list(mut l: Level) -> Result<()> { ); r.add_str( "ack", - &del.ack.map(|n| n.to_string()).unwrap_or_else(|| "-".to_string()), + del.ack.map(|n| n.to_string()).unwrap_or_else(|| "-".to_string()), ); let seq = del.seq; @@ -371,10 +371,10 @@ async fn do_check_suite_list(mut l: Level) -> Result<()> { for suite in l.context().db().list_check_suites()? { let mut r = Row::default(); - r.add_str("id", &suite.id.to_string()); + r.add_str("id", suite.id.to_string()); r.add_u64("ghid", suite.github_id as u64); r.add_u64("repo", suite.repo as u64); - r.add_str("state", &format!("{:?}", suite.state)); + r.add_str("state", format!("{:?}", suite.state)); r.add_str("ssha", &suite.head_sha[0..SHORT_SHA_LEN]); r.add_str("sha", suite.head_sha); r.add_str("branch", suite.head_branch.as_deref().unwrap_or("-")); @@ -407,14 +407,14 @@ async fn do_check_suite_runs(mut l: Level) -> Result<()> { for run in l.context().db().list_check_runs_for_suite(csid)? { let mut r = Row::default(); - r.add_str("id", &run.id.to_string()); + r.add_str("id", run.id.to_string()); r.add_flags("flags") .flag('A', run.active) .flag('F', run.flushed) .build(); r.add_str("active", if run.active { "yes" } else { "no" }); r.add_str("flushed", if run.flushed { "yes" } else { "no" }); - r.add_str("variety", &run.variety.to_string()); + r.add_str("variety", run.variety.to_string()); r.add_str("name", &run.name); t.add_row(r); diff --git a/github/server/src/http.rs b/github/server/src/http.rs index 2bd1f93..68c9624 100644 --- a/github/server/src/http.rs +++ b/github/server/src/http.rs @@ -359,7 +359,7 @@ async fn details( Ok(hyper::Response::builder() .status(hyper::StatusCode::OK) .header(hyper::header::CONTENT_TYPE, "text/html; charset=utf-8") - .header(hyper::header::CONTENT_LENGTH, out.as_bytes().len()) + .header(hyper::header::CONTENT_LENGTH, out.len()) .body(Body::from(out))?) } @@ -831,7 +831,7 @@ async fn status_impl( Ok(hyper::Response::builder() .status(hyper::StatusCode::OK) .header(hyper::header::CONTENT_TYPE, "text/html; charset=utf-8") - .header(hyper::header::CONTENT_LENGTH, out.as_bytes().len()) + .header(hyper::header::CONTENT_LENGTH, out.len()) .body(Body::from(out))?) } @@ -1020,7 +1020,7 @@ async fn branch_to_commit( Ok(hyper::Response::builder() .status(hyper::StatusCode::OK) .header(hyper::header::CONTENT_TYPE, "text/plain") - .header(hyper::header::CONTENT_LENGTH, body.as_bytes().len()) + .header(hyper::header::CONTENT_LENGTH, body.len()) .body(body.into())?) } diff --git a/github/server/src/main.rs b/github/server/src/main.rs index a274f65..17616fc 100644 --- a/github/server/src/main.rs +++ b/github/server/src/main.rs @@ -3,6 +3,7 @@ */ #![allow(clippy::vec_init_then_push)] +#![allow(dead_code)] use anyhow::{anyhow, bail, Context, Result}; use base64::Engine; @@ -300,7 +301,7 @@ impl App { fn buildomat(&self, repo: &Repository) -> buildomat_client::Client { buildomat_client::ClientBuilder::new(&self.config.buildomat.url) .bearer_token(&self.config.buildomat.token) - .delegated_user(&self.buildomat_username(repo)) + .delegated_user(self.buildomat_username(repo)) .build() .unwrap() } diff --git a/github/server/src/variety/basic.rs b/github/server/src/variety/basic.rs index 750d6bf..7126bca 100644 --- a/github/server/src/variety/basic.rs +++ b/github/server/src/variety/basic.rs @@ -8,7 +8,9 @@ use buildomat_client::types::{DependSubmit, JobOutput}; use buildomat_common::*; use buildomat_download::PotentialRange; use buildomat_github_database::{types::*, Database}; -use buildomat_jobsh::variety::basic::{output_sse, output_table, BasicConfig}; +use buildomat_jobsh::variety::basic::{ + output_sse, output_table, BasicConfig, BasicConfigPublish, +}; use chrono::SecondsFormat; use dropshot::Body; use futures::{FutureExt, StreamExt, TryStreamExt}; @@ -678,11 +680,11 @@ pub(crate) async fn run( * We pass several GitHub-specific environment variables to tasks in the * job: */ - tb.env("GITHUB_REPOSITORY", &format!("{}/{}", repo.owner, repo.name)); + tb.env("GITHUB_REPOSITORY", format!("{}/{}", repo.owner, repo.name)); tb.env("GITHUB_SHA", &cs.head_sha); if let Some(branch) = cs.head_branch.as_deref() { tb.env("GITHUB_BRANCH", branch); - tb.env("GITHUB_REF", &format!("refs/heads/{}", branch)); + tb.env("GITHUB_REF", format!("refs/heads/{branch}")); } let app0 = app.clone(); @@ -1046,7 +1048,7 @@ pub(crate) async fn artefact( .header(hyper::header::CONTENT_TYPE, "text/html") .header(hyper::header::CONTENT_LENGTH, md.len()) .body(Body::wrap(StreamBody::new( - stream.map_ok(|b| Frame::data(b)), + stream.map_ok(Frame::data), )))?, )); } @@ -1110,7 +1112,7 @@ pub(crate) async fn artefact( .header(hyper::header::CONTENT_TYPE, ct) .header(hyper::header::CONTENT_LENGTH, cl) .body(Body::wrap(StreamBody::new( - backend.into_inner_stream().map_ok(|b| Frame::data(b)), + backend.into_inner_stream().map_ok(Frame::data), )))?, )); } @@ -1257,7 +1259,7 @@ pub(crate) async fn details( &bm, &job, local_time, - format!("./{}/live", cr.id.to_string()), + format!("./{}/live", cr.id), ) .await?; } @@ -1315,6 +1317,7 @@ pub(crate) async fn cancel( pub mod test { use super::*; use buildomat_github_testdata::*; + use buildomat_jobsh::jobfile::JobFile; #[test] fn basic_parse_basic() -> Result<()> { diff --git a/github/testdata/Cargo.toml b/github/testdata/Cargo.toml index 3c22ed0..4c41c4a 100644 --- a/github/testdata/Cargo.toml +++ b/github/testdata/Cargo.toml @@ -10,6 +10,7 @@ doctest = false [dependencies] buildomat-common = { path = "../../common" } buildomat-github-database = { path = "../database" } +buildomat-jobsh = { path = "../../jobsh" } anyhow = { workspace = true } serde = { workspace = true } diff --git a/github/testdata/src/lib.rs b/github/testdata/src/lib.rs index 8e53ca4..a78e725 100644 --- a/github/testdata/src/lib.rs +++ b/github/testdata/src/lib.rs @@ -1,4 +1,4 @@ -use buildomat_github_database::types::{CheckRunVariety, JobFile}; +use buildomat_jobsh::jobfile::{JobFile, Variety}; pub fn disabled0() -> (String, String) { let path = ".github/buildomat/jobs/disabled0.sh"; @@ -106,7 +106,7 @@ pub fn real0() -> (String, String, Option) { let expect = Some(JobFile { path: path.to_string(), name: "helios / build TUF repo".to_string(), - variety: CheckRunVariety::Basic, + variety: Variety::Basic, config: serde_json::json!({ "access_repos": [ "oxidecomputer/amd-apcb", diff --git a/jobsh/src/jobfile.rs b/jobsh/src/jobfile.rs index e6d0386..8ecb6b6 100644 --- a/jobsh/src/jobfile.rs +++ b/jobsh/src/jobfile.rs @@ -280,3 +280,282 @@ impl JobFileSet { Ok(jobfiles) } } + +#[cfg(test)] +mod test { + use super::*; + + fn make_job(name: &str) -> String { + format!( + "#!/bin/bash\n\ + #: name = \"{name}\"\n\ + #: variety = \"basic\"\n\ + echo hello\n" + ) + } + + fn make_job_with_dep(name: &str, dep_name: &str, dep_job: &str) -> String { + format!( + "#!/bin/bash\n\ + #: name = \"{name}\"\n\ + #: variety = \"basic\"\n\ + #: [dependencies.{dep_name}]\n\ + #: job = \"{dep_job}\"\n\ + echo hello\n" + ) + } + + #[test] + fn jobfileset_single_job() { + let mut jfs = JobFileSet::new(10); + jfs.load(&make_job("test-job"), "jobs/test.sh").unwrap(); + let jobs = jfs.complete().unwrap(); + assert_eq!(jobs.len(), 1); + assert_eq!(jobs[0].name, "test-job"); + } + + #[test] + fn jobfileset_multiple_jobs() { + let mut jfs = JobFileSet::new(10); + jfs.load(&make_job("job-a"), "jobs/a.sh").unwrap(); + jfs.load(&make_job("job-b"), "jobs/b.sh").unwrap(); + jfs.load(&make_job("job-c"), "jobs/c.sh").unwrap(); + let jobs = jfs.complete().unwrap(); + assert_eq!(jobs.len(), 3); + // Jobs should be sorted by name + assert_eq!(jobs[0].name, "job-a"); + assert_eq!(jobs[1].name, "job-b"); + assert_eq!(jobs[2].name, "job-c"); + } + + #[test] + fn jobfileset_duplicate_names() { + let mut jfs = JobFileSet::new(10); + jfs.load(&make_job("same-name"), "jobs/a.sh").unwrap(); + jfs.load(&make_job("same-name"), "jobs/b.sh").unwrap(); + let err = jfs.complete().unwrap_err(); + assert!( + err.to_string().contains("used in more than one file"), + "expected duplicate name error, got: {}", + err + ); + } + + #[test] + fn jobfileset_valid_dependency() { + let mut jfs = JobFileSet::new(10); + jfs.load(&make_job("first-job"), "jobs/first.sh").unwrap(); + jfs.load( + &make_job_with_dep("second-job", "needs", "first-job"), + "jobs/second.sh", + ) + .unwrap(); + let jobs = jfs.complete().unwrap(); + assert_eq!(jobs.len(), 2); + } + + #[test] + fn jobfileset_missing_dependency() { + let mut jfs = JobFileSet::new(10); + jfs.load( + &make_job_with_dep("my-job", "needs", "nonexistent-job"), + "jobs/my.sh", + ) + .unwrap(); + let err = jfs.complete().unwrap_err(); + assert!( + err.to_string().contains("not present in the plan"), + "expected missing dependency error, got: {}", + err + ); + } + + #[test] + fn jobfileset_self_dependency_cycle() { + // A job that depends on itself + let content = "\ + #!/bin/bash\n\ + #: name = \"self-ref\"\n\ + #: variety = \"basic\"\n\ + #: [dependencies.me]\n\ + #: job = \"self-ref\"\n\ + echo hello\n"; + let mut jfs = JobFileSet::new(10); + jfs.load(content, "jobs/self.sh").unwrap(); + let err = jfs.complete().unwrap_err(); + assert!( + err.to_string().contains("dependency cycle"), + "expected cycle error, got: {}", + err + ); + } + + #[test] + fn jobfileset_two_job_cycle() { + // A -> B -> A + let job_a = "\ + #!/bin/bash\n\ + #: name = \"job-a\"\n\ + #: variety = \"basic\"\n\ + #: [dependencies.needs_b]\n\ + #: job = \"job-b\"\n\ + echo a\n"; + let job_b = "\ + #!/bin/bash\n\ + #: name = \"job-b\"\n\ + #: variety = \"basic\"\n\ + #: [dependencies.needs_a]\n\ + #: job = \"job-a\"\n\ + echo b\n"; + let mut jfs = JobFileSet::new(10); + jfs.load(job_a, "jobs/a.sh").unwrap(); + jfs.load(job_b, "jobs/b.sh").unwrap(); + let err = jfs.complete().unwrap_err(); + assert!( + err.to_string().contains("dependency cycle"), + "expected cycle error, got: {}", + err + ); + } + + #[test] + fn jobfileset_diamond_dependency_ok() { + // Diamond: D depends on B and C, both depend on A + // This is NOT a cycle and should succeed + let job_a = make_job("job-a"); + let job_b = make_job_with_dep("job-b", "needs", "job-a"); + let job_c = make_job_with_dep("job-c", "needs", "job-a"); + let job_d = "\ + #!/bin/bash\n\ + #: name = \"job-d\"\n\ + #: variety = \"basic\"\n\ + #: [dependencies.needs_b]\n\ + #: job = \"job-b\"\n\ + #: [dependencies.needs_c]\n\ + #: job = \"job-c\"\n\ + echo d\n"; + + let mut jfs = JobFileSet::new(10); + jfs.load(&job_a, "jobs/a.sh").unwrap(); + jfs.load(&job_b, "jobs/b.sh").unwrap(); + jfs.load(&job_c, "jobs/c.sh").unwrap(); + jfs.load(job_d, "jobs/d.sh").unwrap(); + let jobs = jfs.complete().unwrap(); + assert_eq!(jobs.len(), 4); + } + + #[test] + fn jobfileset_non_sh_extension() { + let mut jfs = JobFileSet::new(10); + let err = jfs.load(&make_job("test"), "jobs/test.txt").unwrap_err(); + assert!( + err.to_string().contains("unexpected item"), + "expected extension error, got: {}", + err + ); + } + + #[test] + fn jobfileset_too_many_jobs() { + let mut jfs = JobFileSet::new(2); + jfs.load(&make_job("job-1"), "jobs/1.sh").unwrap(); + jfs.load(&make_job("job-2"), "jobs/2.sh").unwrap(); + jfs.load(&make_job("job-3"), "jobs/3.sh").unwrap(); + let err = jfs.load(&make_job("job-4"), "jobs/4.sh").unwrap_err(); + assert!( + err.to_string().contains("too many job files"), + "expected too many jobs error, got: {}", + err + ); + } + + #[test] + fn jobfile_missing_shebang() { + let content = "\ + #: name = \"test\"\n\ + #: variety = \"basic\"\n\ + echo hello\n"; + let err = JobFile::parse_content_at_path(content, "test.sh").unwrap_err(); + assert!( + err.to_string().contains("interpreter line"), + "expected shebang error, got: {}", + err + ); + } + + #[test] + fn jobfile_misspelled_enable_enabled() { + let content = "\ + #!/bin/bash\n\ + #: name = \"test\"\n\ + #: variety = \"basic\"\n\ + #: enabled = true\n\ + echo hello\n"; + let err = JobFile::parse_content_at_path(content, "test.sh").unwrap_err(); + assert!( + err.to_string().contains("use \"enable\""), + "expected misspelling error, got: {}", + err + ); + } + + #[test] + fn jobfile_misspelled_enable_disabled() { + let content = "\ + #!/bin/bash\n\ + #: name = \"test\"\n\ + #: variety = \"basic\"\n\ + #: disabled = true\n\ + echo hello\n"; + let err = JobFile::parse_content_at_path(content, "test.sh").unwrap_err(); + assert!( + err.to_string().contains("use \"enable\""), + "expected misspelling error, got: {}", + err + ); + } + + #[test] + fn jobfile_with_dependencies() { + let content = "\ + #!/bin/bash\n\ + #: name = \"dependent-job\"\n\ + #: variety = \"basic\"\n\ + #: [dependencies.build]\n\ + #: job = \"build-job\"\n\ + #: [dependencies.test]\n\ + #: job = \"test-job\"\n\ + echo hello\n"; + let jf = JobFile::parse_content_at_path(content, "test.sh") + .unwrap() + .unwrap(); + assert_eq!(jf.dependencies.len(), 2); + assert_eq!(jf.dependencies.get("build").unwrap().job, "build-job"); + assert_eq!(jf.dependencies.get("test").unwrap().job, "test-job"); + } + + #[test] + fn jobfile_empty_content() { + // Empty file has no lines, so lines.next() returns None and + // TOML parsing fails due to missing required fields + let content = ""; + let err = JobFile::parse_content_at_path(content, "test.sh").unwrap_err(); + assert!( + err.to_string().contains("TOML front matter"), + "expected TOML parsing error for empty file, got: {}", + err + ); + } + + #[test] + fn jobfile_only_shebang() { + // Just a shebang with no frontmatter should fail TOML parsing + let content = "#!/bin/bash\n"; + let err = JobFile::parse_content_at_path(content, "test.sh").unwrap_err(); + assert!( + err.to_string().contains("TOML front matter"), + "expected TOML parsing error, got: {}", + err + ); + } +} diff --git a/jobsh/src/variety/basic.rs b/jobsh/src/variety/basic.rs index 92aec58..a5c32e6 100644 --- a/jobsh/src/variety/basic.rs +++ b/jobsh/src/variety/basic.rs @@ -95,7 +95,7 @@ pub async fn output_table( for ev in page { minseq = ev.seq + 1; - payload_size += ev.payload.as_bytes().len(); + payload_size += ev.payload.len(); events.push(ev); if payload_size > PAYLOAD_SIZE_MAX { diff --git a/server/src/db/mod.rs b/server/src/db/mod.rs index 9c78fb8..8636a6a 100644 --- a/server/src/db/mod.rs +++ b/server/src/db/mod.rs @@ -2361,7 +2361,7 @@ impl Database { */ let max_val_count = 100; let max_val_kib = 10; - if value.as_bytes().len() > max_val_kib * 1024 { + if value.len() > max_val_kib * 1024 { conflict!("maximum value size is {max_val_kib}KiB"); } diff --git a/server/src/main.rs b/server/src/main.rs index dc52d40..d9992c6 100644 --- a/server/src/main.rs +++ b/server/src/main.rs @@ -4,6 +4,7 @@ #![allow(clippy::many_single_char_names)] #![allow(clippy::too_many_arguments)] +#![allow(dead_code)] use std::collections::VecDeque; use std::io::{Seek, Write}; diff --git a/server/src/workers.rs b/server/src/workers.rs index 5fdf69e..48c4f4d 100644 --- a/server/src/workers.rs +++ b/server/src/workers.rs @@ -163,10 +163,8 @@ pub(crate) async fn worker_cleanup(log: Logger, c: Arc) -> Result<()> { info!(log, "starting worker liveness checks"); liveness_checks = true; } - } else { - if let Err(e) = worker_liveness_one(&log, &c).await { - error!(log, "worker liveness task error: {:?}", e); - } + } else if let Err(e) = worker_liveness_one(&log, &c).await { + error!(log, "worker liveness task error: {e:?}"); } tokio::time::sleep(delay).await; diff --git a/sse/src/lib.rs b/sse/src/lib.rs index 7d9b968..d69aa27 100644 --- a/sse/src/lib.rs +++ b/sse/src/lib.rs @@ -111,7 +111,7 @@ impl ServerSentEvents { ))))?) } - pub fn build_event(&self) -> EventBuilder { + pub fn build_event(&self) -> EventBuilder<'_> { EventBuilder { sse: self, id: None, event: None, data: None } } diff --git a/types/src/config.rs b/types/src/config.rs index c3f4afe..fc69d14 100644 --- a/types/src/config.rs +++ b/types/src/config.rs @@ -342,6 +342,10 @@ mod test { Some(Inheritable::Inherit(ConfigFileInherit { inherit: false })) } + fn yes_inherit() -> Option { + Some(Inheritable::Inherit(ConfigFileInherit { inherit: true })) + } + #[test] fn base_with_removals() -> Result<()> { let base = ConfigFileDiag { @@ -379,4 +383,91 @@ mod test { assert_eq!(loaded, expect); Ok(()) } + + #[test] + fn explicit_inherit_true() -> Result<()> { + // When "inherit: true" is specified, values should be inherited from base + let base = ConfigFileDiag { + root_password_hash: Some(Inheritable::String("password".into())), + root_authorized_keys: Some(Inheritable::String("keys".into())), + dump_to_rpool: Some(Inheritable::Num(100)), + pre_job_diagnostic_script: Some(Inheritable::String("pre".into())), + post_job_diagnostic_script: Some(Inheritable::String( + "post".into(), + )), + rpool_disable_sync: Some(Inheritable::Bool(true)), + }; + + let over = ConfigFileDiag { + root_password_hash: yes_inherit(), + root_authorized_keys: yes_inherit(), + dump_to_rpool: yes_inherit(), + pre_job_diagnostic_script: yes_inherit(), + post_job_diagnostic_script: yes_inherit(), + rpool_disable_sync: yes_inherit(), + }; + + let loaded = base.apply_overrides(&over).build()?; + + // inherit: true should pass through base values + let expect = FactoryMetadata::V1(FactoryMetadataV1 { + addresses: Default::default(), + root_password_hash: Some("password".to_string()), + root_authorized_keys: Some("keys".to_string()), + dump_to_rpool: Some(100), + pre_job_diagnostic_script: Some("pre".to_string()), + post_job_diagnostic_script: Some("post".to_string()), + rpool_disable_sync: Some(true), + }); + + assert_eq!(loaded, expect); + Ok(()) + } + + #[test] + fn mixed_inherit_and_override() -> Result<()> { + // Some fields inherit, some override, some block inheritance + let base = ConfigFileDiag { + root_password_hash: Some(Inheritable::String("base-password".into())), + root_authorized_keys: Some(Inheritable::String("base-keys".into())), + dump_to_rpool: Some(Inheritable::Num(100)), + pre_job_diagnostic_script: Some(Inheritable::String("base-pre".into())), + post_job_diagnostic_script: Some(Inheritable::String( + "base-post".into(), + )), + rpool_disable_sync: Some(Inheritable::Bool(false)), + }; + + let over = ConfigFileDiag { + // Explicitly inherit from base + root_password_hash: yes_inherit(), + // Override with new value + root_authorized_keys: Some(Inheritable::String("new-keys".into())), + // Block inheritance (no value) + dump_to_rpool: no_inherit(), + // Implicit inherit (None = inherit from base) + pre_job_diagnostic_script: None, + // Override with new value + post_job_diagnostic_script: Some(Inheritable::String( + "new-post".into(), + )), + // Override boolean + rpool_disable_sync: Some(Inheritable::Bool(true)), + }; + + let loaded = base.apply_overrides(&over).build()?; + + let expect = FactoryMetadata::V1(FactoryMetadataV1 { + addresses: Default::default(), + root_password_hash: Some("base-password".to_string()), // inherited + root_authorized_keys: Some("new-keys".to_string()), // overridden + dump_to_rpool: None, // blocked + pre_job_diagnostic_script: Some("base-pre".to_string()), // implicit inherit + post_job_diagnostic_script: Some("new-post".to_string()), // overridden + rpool_disable_sync: Some(true), // overridden + }); + + assert_eq!(loaded, expect); + Ok(()) + } }