mplzik / rnr Goto Github PK

Heya. I've been looking at this with more detail, and I'd like to propose a new package layout that I think it will make things easier to maintain.

At the top level we have the current import path, github.com/mplzik/rnr/golang/pkg/rnr, which contains the Task interface and the Job struct.

Then we have github.com/mplzik/rnr/golang/pkg/rnr/tasks/web with the Server struct for what it currently resides in webserver.go.

Last but not least, we have for each type of class, the following:

• github.com/mplzik/rnr/golang/pkg/rnr/tasks/nested
• github.com/mplzik/rnr/golang/pkg/rnr/tasks/shell
• github.com/mplzik/rnr/golang/pkg/rnr/tasks/callback

Each of these packages rename the existing FooTask into a Task structure, making the code a little less verbose or more familiar across sub-packages, as in:

t1 := nested.New("foo", nested.Options{})

t2 := shell.New("bar", "kubectl", "get", "pods")`

t3 := callback.New("quux", someFunc)

By doing so we remove the need to use prefixes for each type, and it gives more isolation between definitions, code, and tests.

I believe this should be done first before my other proposal in #6.

What do you think?

While looking at the current interface definition for Task I've seen that Task.Poll doesn't accept any argument nor return an error (more on this later).

Then looking at implementations of the interface, I've found that a context could be useful here:

• CallbackTask creates a context.Background(), thus not giving the option to pass a root context that could be cancelled for different reasons.
• ShellTask could instead create the command using exec.CommandContext so we could have finer control over the executed binary.
• NestedTask could pass the context to its children.

Contexts are great and very powerful when used this way. As an example, if the interface was called this way, we could have in our main.go the following piece of code:

func main() {
	ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt)
	defer stop()

	// …create tasks and everything…

	root.Poll(ctx)

	// …continue…
}

And if the user clicks ^C while the program is calling root.Poll it will cancel the context, thus passing this cancellation information down to all processes and we will have better resource management and cancellation, rather than an abrupt termination.

What do you think?

PS: I mention this function also returning an error. I think it could be very useful information to have, for instance, implementations would perhaps return ctx.Err() which might indicate to the caller that the context was cancelled or timed out.