Merge pull request #4 from bittrance/options-validation

Proper validation of cli options

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

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}")]

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 || {

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,25 +27,56 @@ pub struct CliOptions {
     #[clap(long)]
     pub url: Option<String>,
     /// 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)]
     pub action: Option<String>,
     /// Environment variable for action
     #[clap(long)]
     pub environment: Vec<String>,
-    /// 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<Duration>,
-    /// 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<Duration>,
     /// Run once and exit
     #[clap(long)]
     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<GitTaskConfig>,
@@ -69,15 +102,9 @@ fn tasks_from_opts(opts: &CliOptions) -> Result<Vec<GitTask>, GitOpsError> {
 }
 
 pub fn load_tasks(opts: &CliOptions) -> Result<Vec<GitTask>, 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<impl Store, GitOpsError> {
     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: