diff --git a/README.md b/README.md index bb14760..35f0536 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ The plan forward, roughly in falling priority: - [x] Reasonable timeout duration entry (i.e. not serde default secs/nanos) - [x] Errors in scoped blocks should cancel, not wait for watchdog to reach deadline - [ ] allow configuring notification actions -- [ ] proper options validation (e.g. config-file xor url/action) +- [x] proper options validation (e.g. config-file xor url/action) - [ ] specialized notification action to update github status - [ ] new git sha and branch name in action env vars - [ ] changed task config should override state loaded from disk diff --git a/src/errors.rs b/src/errors.rs index 8407192..c257e18 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -13,9 +13,13 @@ pub enum GitOpsError { #[error("Malformed configuration: {0}")] MalformedConfig(serde_yaml::Error), #[error("Provide --url and --action or --config-file")] - ConfigConflict, + ConfigMethodConflict, + #[error("Provide --interval or --once-only")] + ConfigExecutionConflict, #[error("Cannot find directory to store repositories: {0}")] MissingRepoDir(PathBuf), + #[error("Failed to create directory to store repositories: {0}")] + CreateRepoDir(std::io::Error), #[error("Failed to open/create state file: {0}")] StateFile(std::io::Error), #[error("Falied to read state: {0}")] diff --git a/src/main.rs b/src/main.rs index 1c03c0e..a102ac7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,17 +12,7 @@ use std::{collections::HashSet, sync::mpsc::channel, thread::spawn}; fn main() -> Result<(), GitOpsError> { let mut opts = CliOptions::parse(); - if let Some(ref dir) = opts.repo_dir { - if !dir.exists() { - return Err(GitOpsError::MissingRepoDir(dir.clone())); - } - } else { - opts.repo_dir = Some( - tempfile::tempdir() - .map_err(GitOpsError::WorkDir)? - .into_path(), - ); - } + opts.complete()?; let (tx, rx) = channel(); // TODO Handle TERM both here and when running actions spawn(move || { diff --git a/src/opts.rs b/src/opts.rs index 0dfa258..3297cef 100644 --- a/src/opts.rs +++ b/src/opts.rs @@ -10,6 +10,8 @@ use crate::{ task::{GitTask, GitTaskConfig}, }; +const DEFAULT_BRANCH: &str = "main"; + #[derive(Parser)] pub struct CliOptions { /// Path where state is stored @@ -25,7 +27,7 @@ pub struct CliOptions { #[clap(long)] pub url: Option, /// Branch to check out - #[clap(long, default_value = "main")] + #[clap(long, default_value = DEFAULT_BRANCH)] pub branch: String, /// Command to execute on change (passed to /bin/sh) #[clap(long)] @@ -33,10 +35,10 @@ pub struct CliOptions { /// Environment variable for action #[clap(long)] pub environment: Vec, - /// Check repo for changes at this interval + /// Check repo for changes at this interval (e.g. 1h, 30m, 10s) #[arg(long, value_parser = humantime::parse_duration)] pub interval: Option, - /// Max run time for repo fetch plus action in seconds + /// Max run time for repo fetch plus action (e.g. 1h, 30m, 10s) #[arg(long, value_parser = humantime::parse_duration)] pub timeout: Option, /// Run once and exit @@ -44,6 +46,37 @@ pub struct CliOptions { pub once_only: bool, } +impl CliOptions { + pub fn complete(&mut self) -> Result<(), GitOpsError> { + if self.config_file.is_some() { + if self.url.is_some() + || self.branch != DEFAULT_BRANCH + || self.action.is_some() + || !self.environment.is_empty() + { + return Err(GitOpsError::ConfigMethodConflict); + } + } else if self.url.is_none() || self.action.is_none() { + return Err(GitOpsError::ConfigMethodConflict); + } + if self.once_only && self.interval.is_some() { + return Err(GitOpsError::ConfigExecutionConflict); + } + if let Some(ref dir) = self.repo_dir { + if !dir.exists() { + return Err(GitOpsError::MissingRepoDir(dir.clone())); + } + } else { + self.repo_dir = Some( + tempfile::tempdir() + .map_err(GitOpsError::CreateRepoDir)? + .into_path(), + ); + } + Ok(()) + } +} + #[derive(Deserialize)] struct ConfigFile { tasks: Vec, @@ -69,15 +102,9 @@ fn tasks_from_opts(opts: &CliOptions) -> Result, GitOpsError> { } pub fn load_tasks(opts: &CliOptions) -> Result, GitOpsError> { - if opts.action.is_some() || opts.url.is_some() { - if opts.action.is_none() || opts.url.is_none() || opts.config_file.is_some() { - return Err(GitOpsError::ConfigConflict); - } + if opts.url.is_some() { tasks_from_opts(opts) } else { - if opts.config_file.is_none() { - return Err(GitOpsError::ConfigConflict); - } tasks_from_file(opts) } } @@ -86,6 +113,33 @@ pub fn load_store(opts: &CliOptions) -> Result { FileStore::from_file(&opts.state_file) } +#[test] +fn complete_cli_options_no_args() { + let mut opts = CliOptions::parse_from(&["kitops"]); + let res = opts.complete(); + assert!(matches!(res, Err(GitOpsError::ConfigMethodConflict))); +} + +#[test] +fn complete_cli_options_incomplete_args() { + let mut opts = CliOptions::parse_from(&["kitops", "--url", "file:///tmp"]); + let res = opts.complete(); + assert!(matches!(res, Err(GitOpsError::ConfigMethodConflict))); +} + +#[test] +fn complete_cli_options_conflicting_args() { + let mut opts = CliOptions::parse_from(&[ + "kitops", + "--config-file", + "foo.yaml", + "--url", + "file:///tmp", + ]); + let res = opts.complete(); + assert!(matches!(res, Err(GitOpsError::ConfigMethodConflict))); +} + #[test] fn minimum_config() { let config = r#"tasks: