Merge pull request #103 from snowplow/release/0.4.1

Release/0.4.1
snowplow-archive · Mar 16, 2017 · b26a70f · b26a70f
2 parents 357000b + ba43c92
commit b26a70f
Show file tree

Hide file tree

Showing 4 changed files with 99 additions and 50 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,3 +1,8 @@
+Version 0.4.1 (2017-03-16)
+--------------------------
+Fix panic!(..) on logging initialization error (#100)
+Fix behaviour on dot --overwrite (#97)
+
 Version 0.4.0 (2016-12-22)
 --------------------------
 Lock version for rust-mustache to 0.7.0 (#90)

diff --git a/Cargo.toml b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "factotum"
-version = "0.4.0"
+version = "0.4.1"
 authors = ["Ed Lewis <[email protected]>", "Josh Beemster <[email protected]>"]
 
 [dependencies]

diff --git a/README.md b/README.md
@@ -1,6 +1,6 @@
 # Factotum
 
-[![Build Status](https://travis-ci.org/snowplow/factotum.svg?branch=master)](https://travis-ci.org/snowplow/factotum) [![Release 0.4.0](http://img.shields.io/badge/release-0.4.0-blue.svg?style=flat)](https://github.com/snowplow/factotum/releases) [![Apache License 2.0](http://img.shields.io/badge/license-Apache--2-blue.svg?style=flat)](http://www.apache.org/licenses/LICENSE-2.0)
+[![Build Status](https://travis-ci.org/snowplow/factotum.svg?branch=master)](https://travis-ci.org/snowplow/factotum) [![Release 0.4.1](http://img.shields.io/badge/release-0.4.1-blue.svg?style=flat)](https://github.com/snowplow/factotum/releases) [![Apache License 2.0](http://img.shields.io/badge/license-Apache--2-blue.svg?style=flat)](http://www.apache.org/licenses/LICENSE-2.0)
 
 A dag running tool designed for efficiently running complex jobs with non-trivial dependency trees. 
 
@@ -15,8 +15,8 @@ A dag running tool designed for efficiently running complex jobs with non-trivia
 Assuming you're running **64 bit Linux**: 
 
 ```{bash}
-wget https://bintray.com/artifact/download/snowplow/snowplow-generic/factotum_0.4.0_linux_x86_64.zip
-unzip factotum_0.4.0_linux_x86_64.zip
+wget https://bintray.com/artifact/download/snowplow/snowplow-generic/factotum_0.4.1_linux_x86_64.zip
+unzip factotum_0.4.1_linux_x86_64.zip
 ./factotum --version
 ```
 

diff --git a/src/main.rs b/src/main.rs
@@ -52,6 +52,7 @@ use std::net;
 #[cfg(test)]
 use std::fs::File;
 use std::collections::HashMap;
+use std::error::Error;
 
 mod factotum;
 
@@ -340,7 +341,7 @@ fn parse_file_and_execute(factfile: &str,
                           env: Option<String>,
                           start_from: Option<String>,
                           webhook_url: Option<String>,
-                          job_tags: Option<HashMap<String,String>>)
+                          job_tags: Option<HashMap<String, String>>)
                           -> i32 {
     parse_file_and_execute_with_strategy(factfile,
                                          env,
@@ -357,7 +358,7 @@ fn parse_file_and_execute_with_strategy<F>(factfile: &str,
                                            strategy: F,
                                            override_result_map: OverrideResultMappings,
                                            webhook_url: Option<String>,
-                                           job_tags: Option<HashMap<String,String>>)
+                                           job_tags: Option<HashMap<String, String>>)
                                            -> i32
     where F: Fn(&str, &mut Command) -> RunResult + Send + Sync + 'static + Copy
 {
@@ -493,12 +494,23 @@ fn parse_file_and_execute_with_strategy<F>(factfile: &str,
 }
 
 fn write_to_file(filename: &str, contents: &str, overwrite: bool) -> Result<(), String> {
-    let mut f = match OpenOptions::new()
-        .write(true)
-        .create_new(!overwrite)
-        .open(filename) {
-        Ok(f) => f,
-        Err(io) => return Err(format!("couldn't create file '{}' ({})", filename, io)),        
+    let mut f = if overwrite {
+        match OpenOptions::new()
+            .write(true)
+            .create(true)
+            .truncate(true)
+            .open(filename) {
+            Ok(f) => f,
+            Err(io) => return Err(format!("couldn't create file '{}' ({})", filename, io)),        
+        }
+    } else {
+        match OpenOptions::new()
+            .write(true)
+            .create_new(true)
+            .open(filename) {
+            Ok(f) => f,
+            Err(io) => return Err(format!("couldn't create file '{}' ({})", filename, io)),        
+        }
     };
 
     match f.write_all(contents.as_bytes()) {
@@ -518,38 +530,37 @@ fn is_valid_url(url: &str) -> Result<(), String> {
     }
 }
 
-fn get_constraint_map(constraints: &Vec<String>) -> HashMap<String,String> {
+fn get_constraint_map(constraints: &Vec<String>) -> HashMap<String, String> {
     get_tag_map(constraints)
 }
 
 fn is_valid_host(host: &str) -> Result<(), String> {
     if host == "*" {
-        return Ok(())
+        return Ok(());
     }
 
     let os_hostname = try!(gethostname_safe().map_err(|e| e.to_string()));
 
-    if host == os_hostname { 
-        return Ok(()) 
+    if host == os_hostname {
+        return Ok(());
     }
 
     let external_addrs = try!(get_external_addrs().map_err(|e| e.to_string()));
-    let host_addrs = try!(dns_lookup::lookup_host(&host).map_err(
-      |_| "could not find any IPv4 addresses for the supplied hostname"
-    ));
+    let host_addrs = try!(dns_lookup::lookup_host(&host)
+        .map_err(|_| "could not find any IPv4 addresses for the supplied hostname"));
 
     for host_addr in host_addrs {
         if let Ok(good_host_addr) = host_addr {
             if external_addrs.iter().any(|external_addr| external_addr.ip() == good_host_addr) {
-                return Ok(())
+                return Ok(());
             }
         }
     }
 
     Err("failed to match any of the interface addresses to the found host addresses".into())
 }
 
-extern {
+extern "C" {
     pub fn gethostname(name: *mut libc::c_char, size: libc::size_t) -> libc::c_int;
 }
 
@@ -559,9 +570,7 @@ fn gethostname_safe() -> Result<String, String> {
 
     let ptr = buf.as_mut_slice().as_mut_ptr();
 
-    let err = unsafe {
-        gethostname(ptr as *mut libc::c_char, len as libc::size_t)
-    } as libc::c_int;
+    let err = unsafe { gethostname(ptr as *mut libc::c_char, len as libc::size_t) } as libc::c_int;
 
     match err {
         0 => {
@@ -577,9 +586,10 @@ fn gethostname_safe() -> Result<String, String> {
             }
             unsafe { buf.set_len(_real_len) }
             Ok(String::from_utf8_lossy(buf.as_slice()).into_owned())
-        },
+        }
         _ => {
-            Err("could not get hostname from system; cannot compare against supplied hostname".into())
+            Err("could not get hostname from system; cannot compare against supplied hostname"
+                .into())
         }
     }
 }
@@ -598,26 +608,28 @@ fn get_external_addrs() -> Result<Vec<net::SocketAddr>, String> {
     }
 
     if external_addrs.len() == 0 {
-        Err("could not find any non-loopback IPv4 addresses in the network interfaces; do you have a working network interface card?".into())
+        Err("could not find any non-loopback IPv4 addresses in the network interfaces; do you \
+             have a working network interface card?"
+            .into())
     } else {
         Ok(external_addrs)
     }
 }
 
-fn get_tag_map(args: &Vec<String>) -> HashMap<String,String> {
-    let mut arg_map: HashMap<String,String> = HashMap::new();
+fn get_tag_map(args: &Vec<String>) -> HashMap<String, String> {
+    let mut arg_map: HashMap<String, String> = HashMap::new();
 
     for arg in args.iter() {
         let split = arg.split(",").collect::<Vec<&str>>();
         if split.len() >= 2 && split[0].trim().is_empty() == false {
-           let key = split[0].trim().to_string();
-           let value = split[1..].join("").trim().to_string();
-           arg_map.insert(key, value);
+            let key = split[0].trim().to_string();
+            let value = split[1..].join("").trim().to_string();
+            arg_map.insert(key, value);
         } else if split.len() == 1 && split[0].trim().is_empty() == false {
-           let key = split[0].trim().to_string();
-           let value = "".to_string();
-           arg_map.insert(key,value);
-        } 
+            let key = split[0].trim().to_string();
+            let value = "".to_string();
+            arg_map.insert(key, value);
+        }
     }
 
     arg_map
@@ -650,30 +662,51 @@ fn test_tag_map() {
     assert_eq!(with_comma, expected_comma);
 }
 
-fn get_log_config() -> Result<log4rs::config::Config, log4rs::config::Errors> {
-    let file_appender = log4rs::appender::FileAppender::builder(".factotum/factotum.log").build();
+fn get_log_config() -> Result<log4rs::config::Config, String> {    
+    let file_appender = match log4rs::appender::FileAppender::builder(".factotum/factotum.log").build() {
+        Ok(fa) => fa,
+        Err(e) => {
+            let cwd = env::current_dir().expect("Unable to get current working directory");
+            let expanded_path = format!("{}{}{}", cwd.display(), std::path::MAIN_SEPARATOR, ".factotum/factotum.log");
+            return Err(format!("couldn't create logfile appender to '{}'. Reason: {}", expanded_path, e.description()));
+        }
+    };
+
     let root = log4rs::config::Root::builder(log::LogLevelFilter::Info)
         .appender("file".to_string());
 
     log4rs::config::Config::builder(root.build())
         .appender(log4rs::config::Appender::builder("file".to_string(),
-                                                    Box::new(file_appender.unwrap()))
-            .build())
-        .build()
+                                                    Box::new(file_appender)).build())
+        .build().map_err(|e| format!("error setting logging. Reason: {}", e.description()))
 }
 
-fn init_logger() {
-    fs::create_dir(".factotum").ok();
-    let log_config = get_log_config();
-    log4rs::init_config(log_config.unwrap()).unwrap();
+fn init_logger() -> Result<(), String> {
+    match fs::create_dir(".factotum") {
+        Ok(_) => (),
+        Err(e) => match e.kind() {
+            std::io::ErrorKind::AlreadyExists => (),
+            _ => {
+                let cwd = env::current_dir().expect("Unable to get current working directory");
+                let expected_path =  format!("{}{}{}{}", cwd.display(), std::path::MAIN_SEPARATOR, ".factotum", std::path::MAIN_SEPARATOR);
+                return Err(format!("unable to create directory '{}' for logfile. Reason: {}", expected_path, e.description()))
+            }
+        }
+    };
+    let log_config = try!(get_log_config());
+    log4rs::init_config(log_config).map_err(|e| format!("couldn't initialize log configuration. Reason: {}", e.description()))
 }
 
 fn main() {
     std::process::exit(factotum())
 }
 
 fn factotum() -> i32 {
-    init_logger();
+
+    if let Err(log) = init_logger() {
+        println!("Log initialization error: {}", log);
+        return PROC_OTHER_ERROR;
+    }
 
     let args: Args = match Docopt::new(USAGE).and_then(|d| d.decode()) {
         Ok(a) => a,
@@ -722,15 +755,16 @@ fn factotum() -> i32 {
             if let Some(host_value) = c_map.get(CONSTRAINT_HOST) {
                 if let Err(msg) = is_valid_host(host_value) {
                     println!("{}",
-                             format!("Warn: the specifed host constraint \"{}\" did not match, no tasks have been executed. Reason: {}",
+                             format!("Warn: the specifed host constraint \"{}\" did not match, \
+                                      no tasks have been executed. Reason: {}",
                                      host_value,
                                      msg)
                                  .yellow());
                     return PROC_SUCCESS;
                 }
             }
         }
-    
+
         if !args.flag_dry_run {
             parse_file_and_execute(&args.arg_factfile,
                                    args.flag_env,
@@ -799,7 +833,11 @@ fn test_is_valid_url() {
 
     match is_valid_url("potato.com/") {
         Ok(_) => panic!("no http/s?"),
-        Err(msg) => assert_eq!(msg, "URL must begin with 'http://' or 'https://' to be used with Factotum webhooks") // this is good
+        Err(msg) => {
+            assert_eq!(msg,
+                       "URL must begin with 'http://' or 'https://' to be used with Factotum \
+                        webhooks")
+        } // this is good
     }
 }
 
@@ -831,6 +869,11 @@ fn test_write_to_file() {
     assert_eq!(contents, "helloworld all");
 
     assert!(fs::remove_file(test_path).is_ok());
+
+    // check that overwrite will also write a new file (https://github.com/snowplow/factotum/issues/97)
+
+    assert!(write_to_file(test_path, "overwrite test", true).is_ok());
+    assert!(fs::remove_file(test_path).is_ok());
 }
 
 #[test]
@@ -1270,7 +1313,8 @@ fn test_is_valid_host() {
     is_valid_host("*").expect("must be Ok() for wildcard");
 
     // Test each external addr is_valid_host
-    let external_addrs = get_external_addrs().expect("get_external_addrs() must return a Ok(Vec<net::SocketAddr>) that is non-empty");
+    let external_addrs = get_external_addrs()
+        .expect("get_external_addrs() must return a Ok(Vec<net::SocketAddr>) that is non-empty");
     for external_addr in external_addrs {
         let ip_str = external_addr.ip().to_string();
         is_valid_host(&ip_str).expect(&format!("must be Ok() for IP {}", &ip_str));