-
Notifications
You must be signed in to change notification settings - Fork 58
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
Implements hooks support #25
Conversation
Would it make sense to, when building the hook allow the dev to pass a custom route to generate?
Which would then generate I can't think of a use case, but it's fairly simple to include. |
Concerning #25 (comment) i don't think we need such an option right now. |
Webhooks are enabled through the `/hooks` route. Building hooks, similar to jobs, is as easy as putting a file in the `hooks/` directory in a Kitto project. The Hook API is very simple and similar to the `Plug.Route` DSL building routes: ```elixir use Kitto.Hooks.DSL hook :github do {:ok, body, _} = read_body conn commits = GitHub.parse_commits_from_hook(body) broadcast! :github_commits, %{commits: commits} end ``` The hook above generates a route at `/hooks/github` listening on all HTTP methods. Hooks include both the `conn` object that routes include as well as `broadcast!/2` to broadcast events to dashboards.
@@ -132,6 +132,27 @@ end | |||
The above will spawn a supervised process which will emit a [server-sent | |||
event](https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events/Using_server-sent_events) with the name `random` every second. | |||
|
|||
## Hooks | |||
|
|||
When jobs don't work, whether you want something more realtime than polling or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When jobs don't work
has a quite negative feeling doesn't it?
|
||
defp load_hooks, do: hook_files |> Enum.each(&Code.load_file/1) | ||
defp hook_files do | ||
[System.cwd, hook_dir, "**/*.{ex,exs}"] |> Path.join |> Path.wildcard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should use Kitto.root
instead of System.cwd
Hooks act like routes in `Plug.Route` and come complete with the `conn` | ||
object for accessing request information. | ||
""" | ||
defmacro __using__(_opts) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add @doc false
and a newline above.
plug :match | ||
plug :dispatch | ||
|
||
match "/:hook" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should hooks be authenticated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that. I would say no. Instead a lot of APIs when sending webhook requests will add a request signature to the HTTP headers of the request, authenticating that the webhook is coming from where it should be coming from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add authentication later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String.to_atom hook
could be DRYied here.
@@ -1,6 +1,18 @@ | |||
defmodule Kitto.TestHelper do | |||
def atomify_map(map) do | |||
for {key, value} <- map, into: %{}, do: {String.to_atom(key), value} | |||
for {key, value} <- map, into: %{} do | |||
{if(is_atom(key), do: key, else: String.to_atom(key)), value} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if is_atom(key), do: ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't work in this context. If I change that line to:
{if is_atom(key), do: key, else: String.to_atom(key), value}
It generates a compile time SyntextError: test/test_helper.exs:4: syntax error before: value
plug :dispatch | ||
|
||
match "/:hook" do | ||
if Kitto.Hooks.hooks[String.to_atom hook] do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better alias both Kitto.Hooks and Kitto.Notifier at the top of the module definition.
All changes that could be implemented, or are not up for further discussion, are added. |
@moduledoc """ | ||
DSL for building Webhook handlers. Define a new Webhook like follows: | ||
|
||
defmodule MyHook do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to define a module. It's documented (and tested) differently elsewhere.
@@ -0,0 +1,3 @@ | |||
use Kitto.Hooks.DSL | |||
|
|||
hook :hello, do: broadcast! :hello, %{text: "Hello World"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests print a warning about variable conn is unused
. We have to do something about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I saw that. Unfortunately, I cannot make the conn
object available in hooks without getting that warning, as not every hook will necessarily use the conn
object.
defmodule Kitto.Hooks.Router do | ||
use Plug.Router | ||
|
||
alias Kitto.{Notifier,Hooks} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] Leave a space after ,
.
defp hook_files do | ||
[hook_dir, "**/*.{ex,exs}"] |> Path.join |> Path.wildcard | ||
end | ||
defp hook_dir, do: Application.get_env(:kitto, :hook_dir, default_hook_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would one want to configure the hooks dir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the reason was to make the logic easier to test, but I thought that maybe someone might want to change it somewhere in the world at some point... maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have it configurable then. I can see how easier it get to test it this way.
@davejlong concerning the unused variable warning, we should go with something like: defmacro hook(name, do: block) do
quote do
# Append the hook to the list of hooks
Kitto.Hooks.register unquote(name), fn(var!(conn)) ->
_ = var!(conn)
unquote(block)
end
end
end Just like phoenix does at: https://github.com/phoenixframework/phoenix/blob/v1.2/lib/phoenix/endpoint.ex#L376 |
Agreed. Do you want to go ahead and add it in?
…On Sat, Nov 26, 2016, 9:18 AM Dimitrios Zorbas ***@***.***> wrote:
@davejlong <https://github.com/davejlong> concerning the unused variable
warning, we should go with something like:
defmacro hook(name, do: block) do
quote do # Append the hook to the list of hooks
Kitto.Hooks.register unquote(name), fn(var!(conn)) ->
_ = var!(conn)
unquote(block)
end
endend
Just like phoenix does at:
https://github.com/phoenixframework/phoenix/blob/v1.2/lib/phoenix/endpoint.ex#L376
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#25 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKs1egYPqkdqkq-etTa7c_NJzanLkVeks5rCD-ugaJpZM4K2fFq>
.
|
I'll work up an example of a Slack slash command using the hooks support, but I think we can go ahead and merge this in. Expect another blog post and PR for the slack command hook later this week. |
Hooks are stored in the `hooks/` directory and are structured as follows: | ||
|
||
```elixir | ||
# File jobs/github.exs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be File: hooks/github.exs
.
@@ -0,0 +1,3 @@ | |||
defmodule Kitto.Hooks.DSLTest do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go to test/fixtures/hooks/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. I should actually remove it. It was a test case, but there's nothing in the DSL that isn't being tested elsewhere.
# Conflicts: # lib/kitto.ex
Will review this tomorrow (2016-12-05). It's the last thing keeping 0.3.0 from being released and it looks good. |
defmodule Kitto.HooksTest do | ||
use ExUnit.Case, async: true | ||
|
||
# setup do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be commented out?
def handle_call({:lookup, hook}, _from, hooks) do | ||
{:reply, Map.fetch(hooks, hook), hooks} | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✂️ Remove newline
defmodule Kitto.Hooks do | ||
@moduledoc """ | ||
Kitto Hooks enable an alternative to Jobs for feeding data into dashboards. | ||
Hooks enable remote services to push data into Kitto using webhooks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hooks enable remote services to push data into Kitto using webhooks.
This sentence is a bit self-referencing.
the root of the application. Hooks can be defined as follows: | ||
|
||
use Kitto.Hooks.DSL | ||
hook :hello do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave a newline above the hook definition as we do with jobs.
use Kitto.Hooks.DSL | ||
hook :hello do | ||
{:ok, body, _} = read_body conn | ||
broadcast! :hello, body |> Poison.decode! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline above broadcast!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think we should also rewrite broadcast!/2
to broadcast!/1
calls.
## Hooks | ||
|
||
If, instead of polling for new data from a data source, you want to act on data | ||
as it changes, hooks are a useful feature for implementing webhooks to feed data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hooks are a useful feature for implementing webhooks
I find this a bit difficult to comprehend. Considering that someone may not be familiar with the concept of hooks, having hooks
and webhooks
in the same sentence can be a bit confusing.
hook :github do | ||
{:ok, body, _} = read_body conn | ||
commits = GitHub.parse_commits_from_hook(body) | ||
broadcast! :github_commits, %{commits: commits} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add newline above.
``` | ||
|
||
The hook generates a route using the atom in the `hook/2` method. The hook above | ||
will listen at `/hooks/github` on any HTTP method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hook above will respond to any HTTP method but actually only works with requests having a JSON body.
@@ -5,3 +5,5 @@ config :kitto, templates_dir: "test/fixtures/views" | |||
config :kitto, default_layout: "layout" | |||
|
|||
config :logger, level: :warn | |||
|
|||
config :kitto, :hook_dir, "test/fixtures/hooks" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we stub this inside the hooks_test?
@@ -85,6 +86,7 @@ defmodule Kitto do | |||
defp children(_env) do | |||
[supervisor(__MODULE__, [], function: :start_server), | |||
supervisor(Kitto.Notifier, []), | |||
worker(Kitto.Hooks, []), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it doesn't depend on any of the other children, i think we should start it last.
@@ -0,0 +1,30 @@ | |||
defmodule Kitto.Hooks.Router do | |||
use Plug.Router |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised credo doesn't nag about the missing @moduledoc
here.
defp hook_files do | ||
[hook_dir, "**/*.{ex,exs}"] |> Path.join |> Path.wildcard | ||
end | ||
defp hook_dir, do: Application.get_env(:kitto, :hook_dir, default_hook_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be hooks_dir
like https://github.com/kittoframework/kitto/blob/master/lib/kitto/runner.ex#L59
|
||
use GenServer | ||
|
||
def start_link do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting it like https://github.com/kittoframework/kitto/blob/master/lib/kitto/runner.ex#L17
with a configurable name allows us to test it independently by spawning a new process (see: https://github.com/kittoframework/kitto/blob/master/test/runner_test.exs#L32) and then disposing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making the server name dynamic is doable, but will take some time. I would suggest waiting until 0.4 if this is necessary for releasing the PR.
### Callbacks | ||
|
||
def handle_call({:lookup, hook}, from, hooks) when is_atom(hook) do | ||
handle_call({:lookup, Atom.to_string(hook)}, from, hooks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace Atom.to_string(hook)
with either "#{hook}"
or hook |> to_string
@doc """ | ||
Registers a new hook into the registry | ||
""" | ||
def register(name, block) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be like https://github.com/kittoframework/kitto/blob/master/lib/kitto/runner.ex#L31
where the first param is a reference to the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that really necessary since the GenServer is always named :hook_registry
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and then I saw your previous comment above.
|
||
hook :slack do | ||
%{"text" => text} = conn.params | ||
broadcast! :slack_message, %{text: text} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline above
# 3. In the setting for the URL use http://[my.kitto.dashboard]/hooks/slack | ||
# | ||
# As with most webhooks, your Kitto API will need to be publicly accessible | ||
# to Slack. You can build verification using the `token` in the URL parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can build verification using the
token
in the URL parameters
It's not entirely clear how the user is supposed to build authentication. We should either include it in the example, or mention it elsewhere (wiki).
@@ -0,0 +1,68 @@ | |||
defmodule Kitto.Hooks do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this module serves as a hook registry why not just name it Kitto.HookRegistry
?
|
||
def init(:ok) do | ||
load_hooks | ||
{:ok, %{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add newline above.
I'm going to close this for right now while I rethink the hook system. Will reopen a PR after I revise the concept to be more unified with the job running system. |
Starting work for #4. The hook API will enable developers to create
exs
files inhooks/
which will auto load. A hook would look like the following:The
slack
hook above will generate a route at/hooks/slack
. The underlying API ofhook/2
will be leveraging thePlug.Route
API so hooks will have access to the normalconn
object just like any other route.variable conn is unused
compiler warning