osquery / osquery-go

Go bindings for osquery
MIT License
388 stars 79 forks source link

update plugin APIs to require function types instead of interfaces #18

Closed groob closed 7 years ago

groob commented 7 years ago

Two major changes worth discussing. I would like to apply the same template to the Config and Call plugins as well if the idea presented here is accepted.

First, I move the logger plugin into plugin/logger The smaller package is easier to grok as it allows for smaller documentation footprint and less type pollution.

Second, I removed the LoggerPlugin interface in favor of a constructor that takes a name and a logfunc.

The resulting changes in the example are almost invisible, but it removes the burden of creating a struct just to implement a Name() method from the user.

screenshot 2017-06-16 17 25 40
groob commented 7 years ago

I have been considering implementing optional methods for Ping and Shutdown, which could be interface asserted on the passed struct and then called if they exist. Granted, this could also be achieved here using the optional argument pattern.

I thought of doing that as well and I believe the functional pattern is actually a better approach to take here because you'd end up with documented functions to pass a Pinger or another custom method. If you're asserting, the only way the caller knows those methods is to read the documentation of the plugin interface.

It's also not necessary to do either. Anyone can create their custom method for Ping and Shutdown as is:

type PingingLogger struct {
    // embedded plugin
    *Plugin
}

func (p *PingingLogger) Ping() osquery.ExtensionStatus {
  // custom logic here 
}

Because the PingingLogger has it's own Ping method, it would be used instead of the embedded one.

groob commented 7 years ago

The primary motivation for me to rewrite to this new style was because the current constructor stutters a bit.

func NewPlugin(takesAPlugin) *returnedImplementationOfPlugin

I agree that most plugin implementations will likely be methods on some struct, but this way of creating plugins does not enforce that requirement.

I'll leave it up to you to decide wether to merge or close this PR, because it might just be a matter of personal preference. The current implementation works, this version just feels cleaner to me.
If you decide not to close the PR, I can refactor move the remaining code over.

groob commented 7 years ago

Any more thoughts on this PR?

I've spent some time working with the updated API over the weekend, and still personally like the layout a lot.

groob commented 7 years ago

updated the readme.