Add new build options (#7)

[nleiva]

Ok, I believe this is closer to what @dbarrosop had in mind :-)

Logger creation doesn't return any error right now, we can look into in it the future if necessary.

log := logger.NewLogrus(false)

file := "/go/src/github.com/nornir-automation/gornir/examples/hosts.yaml"
plugin := inventory.FromYAML{HostsFile: file}
inv, err := plugin.Create()
if err != nil {
	log.Fatal(err)
}

gr := gornir.New().WithInventory(inv).WithLogger(log)

I will add the runner next.

[dbarrosop]

Do we need this interface with this pattern? To me it feels that it's not needed as the creation is independent from Gornir, like the Logger.

[nleiva]

Yeah, probably not right now. I'm just trying to put a stake in the ground for all new Inventory sources, that they should provide a Create function that returns an Inventory.

Also for testing, I want to create a couple of hosts manually and pass that as a list to a Create function, so you end up with an Inventory.

Having said that, I agree with you and is not necessary at this point in time.

[dbarrosop]

I'd prefer calling this just Filter, mostly for consistency; WithSomething attaches that Something to the returned object while Filter applies a transformation (the inventory is still the same, it just changed due to the filter function).

[nleiva]

I want with WithFilter for a couple of reasons:

1. Filter takes a context right now, which I would prefer to avoid until deemed necessary.

2. original.WithFilter(filter) returns a completely new Gornir with a filter applied over the contents of original. original remains unmodified, so we didn't filter it.

3. Consistency across options; if starts with With, it's an option :-)

[dbarrosop]

I am good removing the context for now (I'd remove it in Inventory.Filter and FilterFunc too for consistency) but I am not sure WithFilter is still a consistent name here as it does something different. The other With methods receive attributes that are set in the new object (or options as you say). On the other hand, WithFilter doesn't set an attribute, it applies a transformation on an existing attribute.

Another way of looking at this is that WithInventory and WithLogger (I also want to add WithRunner so it doesn't have to be passed to Gornir.Run each time but no need to do it in this PR) can be called in any order, that's not true for WithFilter, again, because it doesn't set an attribute, it applies a transformation on an attribute.

And if that wasn't enough you can basically realize that WithFilter is just a shorthand for gr.WithInventory(gr.Inventory.Filter(filterFunc)). Filter is also consistent with Inventory.Filter.

Anyway, don't want to block this PR on this discussion so if you are still not convinced we can merge it anyway after removing the context everywhere and rehash it on a different PR :)

[nleiva]

I'll remove context from Filter (and Inventory.Filter, FilterFunc), plus get rid of WithFilter, then merge.

I'll add WithRunner in another PR.

[dbarrosop]

we need to update doc.go as well

[nleiva]

Good catch, will address this.

[dbarrosop]

I am thinking that maybe New should require inv and log as parameters as they are both mandatory.

[nleiva]

What I thinking is New should provide safe defaults instead. Something like an Empty Inventory and logging from the standard library log package (which doesn't satisfy the Logger interface right now). I can address this in a different PR so it's clearly documented.

This way we keep the syntax clean.

[dbarrosop]

Ok, I have a different view here, I don't want defaults because I think everything should be a plugin. Also, creating as default an empty inventory has no value as it renders Gornir unusable.

Re using the standard log package, it shouldn't be hard adding a plugin that wraps it.

[nleiva]

Yeah, let's keep it simple for now: gornir.New() with no defaults. Let's have a separate discussion on what we want New to become.

[dbarrosop]

WithFilter(filter) is redundant here as it's called a couple of lines below.

[nleiva]

I'd prefer to keep it there, and remove it in the other call, so we end up with:

gr := gornir.New().WithInventory(inv).WithLogger(log).WithFilter(filter)

results, err := gr.RunSync(
		"What's my ip?",
		runner.Parallel(),
		&task.RemoteCommand{Command: "ip addr | grep \\/24 | awk '{ print $2 }'"},
	)

Which in my view, separate the two actions (building the inventory and running the task). But this is just my personal preference.

[dbarrosop]

I want to keep the old pattern to illustrate the builder pattern, people is free to use whatever pattern they prefer in their code but I want to make sure people sees something like this is possible:

gr := gornir.New()
...
results, err := gr.Filter(giveMeSpines).Run(...)
...
results, err = gr.Filter(giveMeLeaves).Run(...)

[nleiva]

Ok, I'll keep that example with this alternative and let the user choose which one they prefer in their code. Thanks.

[dbarrosop]

This is awesome, it's what I was thinking of. Just a couple of minor comments but other than that this is great.

[nleiva]

Ok.

• WithFilter is gone.
• Filter (and Inventory.Filter, FilterFunc) no longer need a context.
• go.doc updated
• Open new issue to determine the future of gornir.New() ( The future of gornir.New()  #24)
• 2_simple_with_filter shows alternative way to filter the Inventory.