Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release/0.4.0 #92

Merged
merged 3 commits into from Dec 22, 2016
Merged

Release/0.4.0 #92

merged 3 commits into from Dec 22, 2016

Conversation

jbeemster
Copy link
Member

Hey @alexanderdean this PR adds support for --constraint="host,{{some_host}}"

At the moment if a host-value set is found we go ahead and attempt to assert this fact. If this fails for whatever reason we exit with return code 3 and print the error message.

Please let me know if you would like to see any different behaviour in this.

@alexanderdean
Copy link
Member

Thanks @jbeemster - I wasn't clear from your comment or a quick scan of the code what happens if the constraint is not matched - not talking about a failure, but rather the no-op where the constraint is not matched...

@jbeemster
Copy link
Member Author

If the constraint is set and it then does not match it prints an error message and exits with code 3. Sounds like this needs to be exit code 0 for a noop!

@alexanderdean
Copy link
Member

Yes - this needs to be exit code 0 or else every cron that doesn't match the constraint is going to generate an error email...

@ninjabear
Copy link
Contributor

hey @jbeemster ! I tried to compile this and got the following error:

$ cargo test
    Updating registry `https://github.com/rust-lang/crates.io-index`
error: failed to select a version for `libc` (required by `factotum`):
all possible versions conflict with previously selected versions of `libc`
  version 0.2.16 in use by libc v0.2.16
  possible versions to select: 0.2.18, 0.2.17

Do you know what it means?

@@ -1,6 +1,6 @@
[package]
name = "factotum"
version = "0.3.0"
version = "0.4.0-rc2"
authors = ["Ed Lewis <support@snowplowanalytics.com>"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add your name in here for internet stardom

const VERSION: &'static str = env!("CARGO_PKG_VERSION");
const USAGE: &'static str =
"
Factotum.

Usage:
factotum run <factfile> [--start=<start_task>] [--env=<env>] [--dry-run] [--no-colour] [--webhook=<url>] [--tag=<tag>]...
factotum run <factfile> [--start=<start_task>] [--env=<env>] [--dry-run] [--no-colour] [--webhook=<url>] [--tag=<tag>]... [--constraint=<constraint>]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to be a repeating item like "tags" is, I think you need the ellipsis (...) on the end of both, e.g.

[--tag=<tag>]... [--constraint=<constraint>]...

I haven't been using docopt long though so I may be incorrect (let me know if so!)

return Ok(())
}

if let Ok(os_hostname) = gethostname_safe() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try! macro lets you escape a lot of this nesting, for example:

let os_hostname = try!(gethostname_safe().map_err(|e| format!("something broke ", e.description));

if host == os_hostname { 
  return Ok(()) 
}

let external_addr = try!(get_external_addr().map_err(|e| .. ); // and so on

More info: https://doc.rust-lang.org/book/error-handling.html#the-try-macro

}
Err("Did not find matching IP for hostname".into())
} else {
Err("Found external IP but failed to list IPs for hostname".into())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other bonus to this is that the Err from before probably has some extra information in it. Your string making sense of it is good - but you can add more to it with that e.g. "Did not find matching IP for hostname (error looking up bla)". That's what the .map_err piece does for you.

}
}

fn get_external_addr() -> Result<net::SocketAddr, ()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you have more than one external address? (it's not too uncommon for this to be the case)

if let Some(host_value) = c_map.get(CONSTRAINT_HOST) {
if let Err(msg) = is_valid_host(host_value) {
println!("{}",
format!("Error: the specifed host constraint \"{}\" is invalid. Reason: {}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does invalid here mean they've input it wrong? Or that we're not running because the constraint isn't met? If it's because the user has entered something invalid, we should return PROC_OTHER_ERROR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here we are checking whether the constraint map has the 'host' key. If it does then we attempt to validate the host value. If this is a success we continue with normal execution but if it doesn't match then we exit silently - as this is not the host we want to execute on.

pub fn gethostname(name: *mut libc::c_char, size: libc::size_t) -> libc::c_int;
}

fn gethostname_safe() -> Result<String, ()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests!

Tests will help me out here when checking behaviour on different operating systems. It's ok if the test just checks that the code runs and returns something sensible (perhaps just Ok(_))

@alexanderdean
Copy link
Member

alexanderdean commented Dec 13, 2016

Hey @jbeemster - I had some feedback as it relates to the release blog post:

However, if I change these to non-valid values:

$ ./factotum run echo.factfile --constraint "host,hostname"
$ Error: the specifed host constraint "hostname" is invalid. Reason: found external IP(s) but failed to list IP(s) for hostname

What this error means is that we were able to list probable IPs for this host but that the hostname provided - when we attempted to get an IP for it - did not match the found list. So it fails to resolve and thus will not run.

If I were to disconnect my WiFi network interface:

$ ./factotum run echo.factfile --constraint "host,192.168.1.12"
$ Error: the specifed host constraint "192.168.1.12" is invalid. Reason: did not find matching IP for hostname

This error means that we could not find a hostname associated with the IP provided.

My thoughts:

  1. I don't think we should use the log level of Error: given we are returning 0 exit code and we expect constraints to not match as part of regular operation
  2. With the specifed host constraint "hostname" is invalid. Reason: found external IP(s) but failed to list IP(s) for hostname, I don't understand this message. What is invalid about the constraint? If the constraint does not match because we could not find a box called hostname, then this is fine - the constraint did not match. There's nothing invalid about the constraint or indeed the hostname
  3. With the specifed host constraint "192.168.1.12" is invalid. Reason: did not find matching IP for hostname, again we are talking about a constraint not passing, nothing is invalid. I also don't understand why the "error" message structure differs from 2. above - why is this message not found external IP(s) but none of them match "192.168.1.12"

@ninjabear
Copy link
Contributor

ninjabear commented Dec 15, 2016

Other things in Factotum might have a use for a Warn message in yellow if you want to add that @jbeemster. It's also important to mention clearly that no tasks have been executed (if they haven't been) imo.

@alexanderdean
Copy link
Member

Yes I think Warn could be good... Agree we should explain nothing has run...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants