diff --git a/Cargo.lock b/Cargo.lock index 203cb89..1a5b374 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -622,7 +622,6 @@ checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" dependencies = [ "block-buffer 0.10.4", "crypto-common 0.1.7", - "subtle", ] [[package]] @@ -928,9 +927,9 @@ checksum = "e629b9b98ef3dd8afe6ca2bd0f89306cec16d43d907889945bc5d6687f2f13c7" [[package]] name = "gitlab-runner" -version = "0.3.0" +version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "58f8bfd3287e403d83d657148efc607c1aa744d0ff7ac3a6dcb8797511d0fe59" +checksum = "35d5c4acce20dcf80e6570687372981c813d67262bd359d277b3d0c677b6515a" dependencies = [ "async-trait", "bytes", @@ -938,14 +937,15 @@ dependencies = [ "flate2", "futures", "glob", - "hmac 0.12.1", + "hmac", + "normalize-path", "parking_lot", "pin-project", "rand 0.10.1", "reqwest", "serde", "serde_json", - "sha2", + "sha2 0.11.0", "tempfile", "thiserror 2.0.18", "tokio", @@ -960,9 +960,9 @@ dependencies = [ [[package]] name = "gitlab-runner-mock" -version = "0.3.0" +version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1b7089aed97831b12b9a80fe2f3767d0211d48bf539acc076654e2cdb00dff99" +checksum = "c83aac627f2dc4a11bb4074638275c3274851efa5c1f891305a7c22f1e9d5fdf" dependencies = [ "futures", "http", @@ -1026,15 +1026,6 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fc0fef456e4baa96da950455cd02c081ca953b141298e41db3fc7e36b1da849c" -[[package]] -name = "hmac" -version = "0.12.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6c49c37c09c17a53d937dfbb742eb3a961d65a994e6bcdcf37e7399d0cc8ab5e" -dependencies = [ - "digest 0.10.7", -] - [[package]] name = "hmac" version = "0.13.0" @@ -1497,7 +1488,7 @@ version = "0.16.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "47bb1e988e6fb779cf720ad431242d3f03167c1b3f2b1aae7f1a94b2495b36ae" dependencies = [ - "sha2", + "sha2 0.10.9", ] [[package]] @@ -1570,6 +1561,12 @@ dependencies = [ "version_check", ] +[[package]] +name = "normalize-path" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f5438dd2b2ff4c6df6e1ce22d825ed2fa93ee2922235cc45186991717f0a892d" + [[package]] name = "nu-ansi-term" version = "0.50.3" @@ -1624,7 +1621,7 @@ dependencies = [ [[package]] name = "obo-cli" -version = "0.1.8" +version = "0.2.0" dependencies = [ "async-trait", "camino", @@ -1706,7 +1703,7 @@ dependencies = [ [[package]] name = "obs-gitlab-runner" -version = "0.1.8" +version = "0.2.0" dependencies = [ "async-trait", "camino", @@ -1755,9 +1752,9 @@ checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" [[package]] name = "open-build-service-api" -version = "0.1.0" +version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3b57803d6cf55358b48dfd66339f33ba936ce5c5c9a7c388889bad93c0c5a880" +checksum = "6c44d5a2ad7c7fc4219b1ceed31e58254ae5a292d344e8f9ab4b95f7828701ef" dependencies = [ "base16ct", "bytes", @@ -1838,7 +1835,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "112d82ceb8c5bf524d9af484d4e4970c9fd5a0cc15ba14ad93dccd28873b0629" dependencies = [ "digest 0.11.3", - "hmac 0.13.0", + "hmac", ] [[package]] @@ -2476,6 +2473,17 @@ dependencies = [ "digest 0.10.7", ] +[[package]] +name = "sha2" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "446ba717509524cb3f22f17ecc096f10f4822d76ab5c0b9822c5f9c284e825f4" +dependencies = [ + "cfg-if", + "cpufeatures 0.3.0", + "digest 0.11.3", +] + [[package]] name = "sharded-slab" version = "0.1.7" @@ -3805,7 +3813,7 @@ dependencies = [ "deflate64", "flate2", "getrandom 0.4.2", - "hmac 0.13.0", + "hmac", "indexmap", "lzma-rust2", "memchr", diff --git a/Cargo.toml b/Cargo.toml index 0e5c2d7..984fffc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,6 +8,11 @@ members = [ "obs-gitlab-runner" ] +[workspace.package] +version = "0.2.0" +edition = "2024" +license = "MIT OR Apache-2.0" + [workspace.dependencies] async-trait = "0.1" camino = "1.2" @@ -16,7 +21,7 @@ clap = { version = "4.6", features = ["default", "derive", "env"] } color-eyre = "0.6" derivative = "2.2" futures-util = "0.3" -open-build-service-api = "0.1.0" +open-build-service-api = "0.1.1" # open-build-service-api = { path = "../open-build-service-rs/open-build-service-api" } open-build-service-mock = "0.1.0" # open-build-service-mock = { path = "../open-build-service-rs/open-build-service-mock" } diff --git a/obo-cli/Cargo.toml b/obo-cli/Cargo.toml index 4d1a7c8..6beec2b 100644 --- a/obo-cli/Cargo.toml +++ b/obo-cli/Cargo.toml @@ -1,9 +1,9 @@ [package] name = "obo-cli" description = "OBS Build Orchestrator — command-line frontend" -version = "0.1.8" -edition = "2024" -license = "MIT OR Apache-2.0" +version.workspace = true +edition.workspace = true +license.workspace = true # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html diff --git a/obo-cli/tests/test_cli.rs b/obo-cli/tests/test_cli.rs index e9a9e0f..555cbf4 100644 --- a/obo-cli/tests/test_cli.rs +++ b/obo-cli/tests/test_cli.rs @@ -69,6 +69,13 @@ impl RunBuilder<'_> for CliRunBuilder { self } + fn saves(self, _patterns: I) -> Self + where + I::Item: AsRef, + { + self + } + fn artifacts(mut self, artifacts: Self::ArtifactsHandle) -> Self { self.dependencies.push(artifacts.0); self @@ -102,6 +109,14 @@ impl RunBuilder<'_> for CliRunBuilder { .env("OBS_SERVER", self.obs_server) .env("OBS_USER", TEST_USER) .env("OBS_PASSWORD", TEST_PASS) + .env( + "OBO_LOG", + if should_enable_trace_logging() { + "obo_core=trace,obo_cli=trace" + } else { + "" + }, + ) .env("OBO_TEST_LOG_TAIL", MONITOR_TEST_LOG_TAIL.to_string()) .env("OBO_TEST_SLEEP_ON_BUILDING_MS", "0") .env( @@ -241,6 +256,7 @@ async fn test_monitor_table( let generate = context .run() .command(generate_command) + .saves(&[DEFAULT_MONITOR_TABLE]) .artifacts(dput.clone()) .go() .await; @@ -271,6 +287,7 @@ async fn test_monitor_table( build_info, &enabled.repo_arch, &script, + &[], success, dput_test, log_test, diff --git a/obo-core/Cargo.toml b/obo-core/Cargo.toml index 6c0f2d1..3689a44 100644 --- a/obo-core/Cargo.toml +++ b/obo-core/Cargo.toml @@ -2,8 +2,8 @@ name = "obo-core" description = "OBS Build Orchestrator — core" version = "0.1.0" -edition = "2024" -license = "MIT OR Apache-2.0" +edition.workspace = true +license.workspace = true [dependencies] async-trait.workspace = true diff --git a/obo-test-support/Cargo.toml b/obo-test-support/Cargo.toml index e6c281c..0e1304d 100644 --- a/obo-test-support/Cargo.toml +++ b/obo-test-support/Cargo.toml @@ -1,8 +1,8 @@ [package] name = "obo-test-support" version = "0.1.0" -edition = "2024" -license = "MIT OR Apache-2.0" +edition.workspace = true +license.workspace = true [dependencies] open-build-service-api.workspace = true diff --git a/obo-tests/Cargo.toml b/obo-tests/Cargo.toml index 4b938f7..443ce63 100644 --- a/obo-tests/Cargo.toml +++ b/obo-tests/Cargo.toml @@ -2,8 +2,8 @@ name = "obo-tests" description = "OBS Build Orchestrator — shared tests for different frontends" version = "0.1.0" -edition = "2024" -license = "MIT OR Apache-2.0" +edition.workspace = true +license.workspace = true [dependencies] async-trait.workspace = true diff --git a/obo-tests/src/lib.rs b/obo-tests/src/lib.rs index 7705212..0c33dcf 100644 --- a/obo-tests/src/lib.rs +++ b/obo-tests/src/lib.rs @@ -17,6 +17,10 @@ use obo_test_support::*; use open_build_service_api as obs; use open_build_service_mock::*; +pub fn should_enable_trace_logging() -> bool { + std::env::var_os("OBO_TEST_TRACE").is_some() +} + #[derive(Clone)] pub struct ObsContext { pub client: obs::Client, @@ -51,6 +55,13 @@ pub trait RunBuilder<'context>: Send + Sync + Sized { self.script(&[cmd.into()]) } + // Sets the patterns of files that will be saved by the given commands, so + // that implementations that need to list out saved artifacts like the + // gitlab runner can verify those lists are correct. + fn saves(self, _patterns: I) -> Self + where + I::Item: AsRef; + fn script(self, cmd: &[String]) -> Self; fn artifacts(self, artifacts: Self::ArtifactsHandle) -> Self; fn timeout(self, timeout: Duration) -> Self; @@ -179,6 +190,7 @@ pub async fn test_dput( let dput = context .run() .command(dput_command.replace(dsc1_file, dsc1_bad_file)) + .saves(&[DEFAULT_BUILD_INFO]) .artifacts(artifacts.clone()) .go() .await; @@ -197,6 +209,7 @@ pub async fn test_dput( let mut dput = context .run() .command(&dput_command) + .saves(&[DEFAULT_BUILD_INFO]) .artifacts(artifacts.clone()) .go() .await; @@ -273,6 +286,7 @@ pub async fn test_dput( dput = context .run() .command(&dput_command) + .saves(&[DEFAULT_BUILD_INFO]) .artifacts(artifacts.clone()) .go() .await; @@ -295,6 +309,7 @@ pub async fn test_dput( dput = context .run() .command(format!("{dput_command} --rebuild-if-unchanged")) + .saves(&[DEFAULT_BUILD_INFO]) .artifacts(artifacts.clone()) .go() .await; @@ -399,6 +414,7 @@ pub async fn test_monitoring( build_info: &ObsBuildInfo, repo: &RepoArch, script: &[String], + artifact_paths: &[String], success: bool, dput_test: DputTest, log_test: MonitorLogTest, @@ -504,6 +520,7 @@ pub async fn test_monitoring( let monitor = context .run() .script(script) + .saves(artifact_paths) .artifacts(dput.clone()) .timeout(MONITOR_TEST_OLD_STATUS_SLEEP_DURATION * 20) .go() @@ -529,7 +546,19 @@ pub async fn test_monitoring( ); assert_eq!( - log.contains(&log_contents), + // If trace-level logging is enabled, and the implementation mixes + // together script output with the trace logs (i.e. the CLI), then our + // rather short test log *probably* ended up in the log output at some + // point. In that case, make sure to require a leading newline, so it + // should only match if the logs were explicitly printed on its own + // line(s) vs the debug/trace-level logs that have a header. (This is + // very janky, but it *only* applies in an explicitly opt-in case, so if + // anything actually breaks it's not a huge deal.) + if should_enable_trace_logging() { + log.contains(&format!("\n{log_contents}")) + } else { + log.contains(&log_contents) + }, !success && log_test == MonitorLogTest::Short ); diff --git a/obs-gitlab-runner/Cargo.toml b/obs-gitlab-runner/Cargo.toml index 36dc964..4755081 100644 --- a/obs-gitlab-runner/Cargo.toml +++ b/obs-gitlab-runner/Cargo.toml @@ -1,8 +1,8 @@ [package] name = "obs-gitlab-runner" -version = "0.1.8" -edition = "2024" -license = "MIT OR Apache-2.0" +version.workspace = true +edition.workspace = true +license.workspace = true # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html @@ -13,7 +13,7 @@ clap.workspace = true color-eyre.workspace = true derivative.workspace = true futures-util.workspace = true -gitlab-runner = "0.3.0" +gitlab-runner = "0.3.2" # gitlab-runner = { path = "../../gitlab-runner-rs/gitlab-runner" } obo-core = { path = "../obo-core" } obo-test-support = { path = "../obo-test-support" } @@ -35,8 +35,8 @@ url = "2.5" [dev-dependencies] claims.workspace = true -gitlab-runner-mock = "0.3.0" -# gitlab-runner-mock = { path = "../gitlab-runner-rs/gitlab-runner-mock" } +gitlab-runner-mock = "0.3.2" +# gitlab-runner-mock = { path = "../../gitlab-runner-rs/gitlab-runner-mock" } obo-tests = { path = "../obo-tests" } open-build-service-mock.workspace = true rstest.workspace = true diff --git a/obs-gitlab-runner/chart/Chart.yaml b/obs-gitlab-runner/chart/Chart.yaml index 37d7398..f8b2e89 100644 --- a/obs-gitlab-runner/chart/Chart.yaml +++ b/obs-gitlab-runner/chart/Chart.yaml @@ -15,10 +15,10 @@ type: application # This is the chart version. This version number should be incremented each time you make changes # to the chart and its templates, including the app version. # Versions are expected to follow Semantic Versioning (https://semver.org/) -version: 0.0.1 +version: 0.2.0 # This is the version number of the application being deployed. This version number should be # incremented each time you make changes to the application. Versions are not expected to # follow Semantic Versioning. They should reflect the version the application is using. # It is recommended to use it with quotes. -appVersion: "0.0.1" +appVersion: "v0.2.0" diff --git a/obs-gitlab-runner/chart/values.yaml b/obs-gitlab-runner/chart/values.yaml index c0882d2..1af847a 100644 --- a/obs-gitlab-runner/chart/values.yaml +++ b/obs-gitlab-runner/chart/values.yaml @@ -17,7 +17,7 @@ image: repository: ghcr.io/collabora/obs-gitlab-runner pullPolicy: Always # Overrides the image tag whose default is the chart appVersion. - tag: "0.1.8" + # tag: "v0.2.0" imagePullSecrets: [] nameOverride: "" diff --git a/obs-gitlab-runner/src/handler.rs b/obs-gitlab-runner/src/handler.rs index e8fb5d0..1505cd5 100644 --- a/obs-gitlab-runner/src/handler.rs +++ b/obs-gitlab-runner/src/handler.rs @@ -453,7 +453,7 @@ mod tests { use rstest::rstest; use tempfile::TempDir; use tracing::{Level, instrument::WithSubscriber}; - use tracing_subscriber::{Layer, Registry, filter::Targets, prelude::*}; + use tracing_subscriber::{Registry, filter::Targets, prelude::*}; use zip::ZipArchive; use crate::logging::GitLabForwarder; @@ -576,6 +576,7 @@ mod tests { variables: HashMap, handler: Box, timeout: Duration, + artifacts_patterns: Vec, } fn create_obs_job_handler_factory( @@ -629,6 +630,15 @@ mod tests { self } + fn saves(mut self, patterns: I) -> Self + where + I::Item: AsRef, + { + self.artifacts_patterns + .extend(patterns.into_iter().map(|x| x.as_ref().to_owned())); + self + } + fn artifacts(mut self, artifacts: Self::ArtifactsHandle) -> Self { self.dependencies.push(artifacts.0); self @@ -662,15 +672,17 @@ mod tests { ); } - builder.add_artifact( - None, - false, - vec!["*".to_owned()], - Some(MockJobArtifactWhen::Always), - "archive".to_owned(), - Some("zip".to_owned()), - None, - ); + if !self.artifacts_patterns.is_empty() { + builder.add_artifact( + None, + false, + self.artifacts_patterns, + Some(MockJobArtifactWhen::Always), + "archive".to_owned(), + Some("zip".to_owned()), + None, + ); + } for dependency in self.dependencies { builder.dependency(dependency); @@ -729,6 +741,7 @@ mod tests { artifacts: HashMap>, ) -> Self::ArtifactsHandle { self.run() + .saves(artifacts.keys()) .job_handler_factory(|_| PutArtifactsHandler { artifacts: Arc::new(artifacts), }) @@ -776,6 +789,7 @@ mod tests { _phantom: PhantomData, }), timeout: EXECUTION_DEFAULT_TIMEOUT, + artifacts_patterns: vec![], } } } @@ -816,9 +830,19 @@ mod tests { .with( tracing_subscriber::fmt::layer() .with_test_writer() - .with_filter( - Targets::new().with_target("obs_gitlab_runner", Level::TRACE), - ), + .with_filter(if should_enable_trace_logging() { + Targets::new() + .with_target("obo_core", Level::TRACE) + .with_target("obs_gitlab_runner", Level::TRACE) + } else { + // If trace-level logging isn't enabled, we want + // the global filter to match what the runner is + // typically run with as closely as possible. + // Since 'tracing' is very complex, this helps + // ensure we don't break something in a way that + // passes tests but fails in practice. + Targets::new().with_default(Level::INFO) + }), ) .with(tracing_error::ErrorLayer::default()) .with(GitLabForwarder::new(layer)), @@ -851,6 +875,7 @@ mod tests { let generate = context .run() .command(generate_command) + .saves(&[DEFAULT_MONITOR_PIPELINE]) .artifacts(dput.clone()) .go() .await; @@ -943,6 +968,10 @@ mod tests { build_info, &enabled.repo_arch, &script, + &artifact_paths + .into_iter() + .map(ToOwned::to_owned) + .collect::>(), success, dput_test, log_test, @@ -1153,6 +1182,7 @@ mod tests { } else { context.run().command("generate-monitor tag") } + .saves(&[DEFAULT_MONITOR_PIPELINE]) .artifacts(build_info); let generate = if test == Some(GenerateMonitorTimeoutLocation::HandlerOption) { diff --git a/obs-gitlab-runner/src/logging.rs b/obs-gitlab-runner/src/logging.rs index 41e11b9..8b277a2 100644 --- a/obs-gitlab-runner/src/logging.rs +++ b/obs-gitlab-runner/src/logging.rs @@ -73,12 +73,32 @@ impl LookupSpan<'span>, F: Fi } fn on_event(&self, event: &Event<'_>, ctx: Context<'_, S>) { + // When Filtered's Layer implementation methods are invoked on an event + // *prior* to on_event() (e.g. enabled()), it will set thread-local + // state indicating whether or not the currently-processed event should + // be enabled. Then, within on_event(), if the event was in fact + // disabled, the thread-local state gets reset, to clear things out for + // the next event to be processed. In other words, Filtered holds hard + // assumptions about the order of methods called when processing an + // event, and it uses these assumptions to manage its state. + // + // This also means that, if we don't *always* call on_event here, those + // assumptions are *violated*, and an event being disabled can carry + // over to the processing of the next event. + self.0.on_event(event, ctx.clone()); + if !is_output_field_set_in_event(event) { - // No special behavior needed, so just forward it as-is. - self.0.on_event(event, ctx); + // No special behavior needed, so just leave things as-is. return; } + // If an event had both the obo *and* gitlab-runner output fields set, + // then it would get logged twice (once by the above on_event, and once + // by our bypass below). This is pretty obvious to avoid, but it's still + // worth making sure that in debug builds (e.g. tests) it doesn't + // happen. + debug_assert!(!self.0.filter().enabled(event.metadata(), &ctx)); + let Some(message) = get_event_message(event) else { return; }; @@ -107,7 +127,8 @@ impl LookupSpan<'span>, F: Fi }; // Bypass the filter completely, because the event was almost certainly - // filtered out in its `enabled` due to lacking `gitlab.output`. + // filtered out due to lacking `gitlab.output` (and we already called + // on_event() for the filter at the start of the function). self.0.inner().on_event(&event, ctx); }