From c4bb071a95078e58a14eb01a8cc874a2af1559bf Mon Sep 17 00:00:00 2001 From: Lars Wirzenius Date: Mon, 9 Dec 2024 07:35:54 +0200 Subject: [PATCH] feat! drop support for limiting the duration of a CI run This adapter is only ever run by the Radicle CI broker, and that can now limit CI run duration itself. Thus having this adapter do it too is pointless duplication. Further, this removes the run time dependency on an external "timeout" command, which turns out not to be portable: it isn't available on Macs, by default. Signed-off-by: Lars Wirzenius --- README.md | 1 - src/config.rs | 17 +---------------- src/engine.rs | 1 - src/run.rs | 19 +------------------ src/runlog.rs | 12 +----------- tests/integration.rs | 22 ---------------------- 6 files changed, 3 insertions(+), 69 deletions(-) diff --git a/README.md b/README.md index 0d0ef68..9c148a6 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,6 @@ The fields are: * `base_url` --- base URL for build logs (mandatory for access from CI broker page) * `state` --- required, directory where per-run directories are stored * `log` --- required, file where native CI should write a log -* `timeout` --- optional, maximum duration of the run, in seconds ## Packaging diff --git a/src/config.rs b/src/config.rs index 4c468bd..4adef50 100644 --- a/src/config.rs +++ b/src/config.rs @@ -4,8 +4,6 @@ use serde::{Deserialize, Serialize}; use crate::logfile::{AdminLog, LogError}; -const DEFAULT_TIMEOUT: usize = 3600; - /// Configuration file for `radicle-native-ci`. #[derive(Debug, Serialize, Deserialize)] #[serde(deny_unknown_fields)] @@ -17,9 +15,6 @@ pub struct Config { /// File where native CI should write a log. pub log: PathBuf, - /// Optional maximum duration of a CI run. - pub timeout: Option, - /// Optional base URL to information about each run. The run ID is /// appended, with a slash if needed. #[serde(skip_serializing_if = "Option::is_none")] @@ -43,17 +38,7 @@ impl Config { pub fn read(filename: &Path) -> Result { let file = std::fs::File::open(filename) .map_err(|e| ConfigError::ReadConfig(filename.into(), e))?; - let mut config: Self = serde_yaml::from_reader(&file) - .map_err(|e| ConfigError::ParseConfig(filename.into(), e))?; - if config.timeout.is_none() { - config.timeout = Some(DEFAULT_TIMEOUT); - } - Ok(config) - } - - /// Return timeout or default. - pub fn timeout(&self) -> usize { - self.timeout.unwrap_or(DEFAULT_TIMEOUT) + serde_yaml::from_reader(&file).map_err(|e| ConfigError::ParseConfig(filename.into(), e)) } /// Return configuration serialized to JSON. diff --git a/src/engine.rs b/src/engine.rs index 07b7b7e..6835212 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -219,7 +219,6 @@ impl Engine { run.set_request(req); run.set_commit(commit); run.set_storage(storage); - run.set_timeout(self.config.timeout()); // Actually run. Examine the run log to decide if the run // succeeded or failed. diff --git a/src/run.rs b/src/run.rs index 609615c..1e78193 100644 --- a/src/run.rs +++ b/src/run.rs @@ -15,9 +15,6 @@ use crate::{ // Exit code to indicate we didn't get one from the process. const NO_EXIT: i32 = 999; -// Exit code for the timeout command to indicate it timed out. -const EXIT_TIMEOUT: i32 = 124; - // Path to the repository's CI run specification. This is relative to // the root of the repository. pub const RUNSPEC_PATH: &str = ".radicle/native.yaml"; @@ -33,7 +30,6 @@ pub struct Run { repo_name: Option, commit: Option, storage: Option, - timeout: Option, src: PathBuf, } @@ -49,7 +45,6 @@ impl Run { request: None, commit: None, storage: None, - timeout: None, src: run_dir.join("src"), }) } @@ -75,13 +70,6 @@ impl Run { self.storage = Some(path.into()); } - /// Optionally set the maximum duration to execute the shell - /// commands specified for a project, in seconds. If not set, a - /// generous built-in default is used. - pub fn set_timeout(&mut self, seconds: usize) { - self.timeout = Some(seconds); - } - /// Run CI on a project. /// /// This runs CI for the project, and then clean up: persist the @@ -131,7 +119,6 @@ impl Run { let request = self.request.clone().ok_or(RunError::Missing("request"))?; let commit = self.commit.ok_or(RunError::Missing("commit"))?; let storage = self.storage.as_ref().ok_or(RunError::Missing("storage"))?; - let timeout = self.timeout.ok_or(RunError::Missing("timeout"))?; // Record metadata in the run log. self.run_log.title("Log from Radicle native CI"); @@ -164,11 +151,7 @@ impl Run { }; let snippet = format!("set -xeuo pipefail\n{}", &runspec.shell); - let timeout = format!("{}", timeout); - let exit = self.runcmd(&["timeout", &timeout, "bash", "-c", &snippet], &src)?; - if exit == EXIT_TIMEOUT { - self.run_log.command_timed_out(); - } + self.runcmd(&["bash", "-c", &snippet], &src)?; Ok(()) } diff --git a/src/runlog.rs b/src/runlog.rs index 6ec8b00..20861b3 100644 --- a/src/runlog.rs +++ b/src/runlog.rs @@ -24,7 +24,6 @@ pub struct RunLog { branch: Option, patch: Option<(Oid, String)>, commands: Vec, - timeout: bool, runspec: Option, runspec_error: Option, } @@ -63,7 +62,7 @@ impl RunLog { } pub fn all_commands_succeeded(&self) -> bool { - self.commands.iter().all(|c| c.exit == 0) && !self.timeout && self.runspec_error.is_none() + self.commands.iter().all(|c| c.exit == 0) && self.runspec_error.is_none() } pub fn runspec(&mut self, runspec: RunSpec) { @@ -96,10 +95,6 @@ impl RunLog { }); } - pub fn command_timed_out(&mut self) { - self.timeout = true; - } - pub fn write(&self) -> Result<(), RunLogError> { let title = self.title.as_ref().ok_or(RunLogError::Missing("title"))?; let rid = self.rid.as_ref().ok_or(RunLogError::Missing("rid"))?; @@ -167,11 +162,6 @@ impl RunLog { body.push_child(error); } - if self.timeout { - let timeout = Element::new(Tag::P).with_text("Last command timed out"); - body.push_child(timeout); - } - if let Some(runspec) = &self.runspec { let text = serde_yaml::to_string(&runspec).map_err(RunLogError::RunSpec)?; body.push_child(toc.push(Element::new(Tag::Span).with_child( diff --git a/tests/integration.rs b/tests/integration.rs index cf1ee18..fbe8b1c 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -36,10 +36,6 @@ use tempfile::{tempdir, TempDir}; use radicle_native_ci::{config::Config, runspec::RunSpec}; -// Default timeout for CI runs. This is deliberately short to keep the -// time to run tests short. -const TIMEOUT: usize = 3; - // The result of running the adapter. #[allow(dead_code)] struct AdapterResult { @@ -367,7 +363,6 @@ impl TestCaseBuilder { let config = Config { state: tmp.join("state"), log: tmp.join("log.html"), - timeout: Some(TIMEOUT), base_url: None, }; let config = serde_yaml::to_string(&config).unwrap(); @@ -376,7 +371,6 @@ impl TestCaseBuilder { let config = Config { state: tmp.join("state"), log: tmp.join("log.html"), - timeout: Some(TIMEOUT), base_url: Some(url.into()), }; let config = serde_yaml::to_string(&config).unwrap(); @@ -704,22 +698,6 @@ fn commit_doesnt_exist() -> TestResult { Ok(()) } -// What does the adapter do if the commands its running take too long? -#[test] -fn command_takes_too_long() -> TestResult { - let result = TestCaseBuilder::new()? - .with_config() - .with_request_for_nonexistent_commit() - .with_shell(&format!("sleep {}", TIMEOUT + 2)) - .build()? - .run()?; - - result.assert_got_exit(1); - result.assert_got_run_id(); - result.assert_got_failure(); - Ok(()) -} - // What does the adapter do if everything goes well? #[test] fn happy_path() -> TestResult {