Api hooks (again)

[aol-nnov]

Revitalized PR #397 which fixes #388
Original PR seems abandoned
All credits go to @hoegaarden, I've just rebased his work on the tip of master and fixed some @smira 's comments from original PR.

Disclamer: I'm not a Go developer at all. I just need hooks in aptly :)
So, I'd be glad if anyone will fix the rest of comments from #397

[aol-nnov]

bummer! tests are failing.. (despite they pass locally) and I don't understand why..
any clues?

[aol-nnov]

Totalitarian language with totalitarian compiler/linter! I'm embarassed...
Btw, @smira one of linter errors (seems it treats warnings as errors) is yours from 211ac05

Still failing, anyway

Exception: Running command aptly mirror create --keyring=aptlytest.gpg pagerduty http://packages.pagerduty.com/pdagent deb/ failed: exit code 1 != 0 (output: Downloading http://packages.pagerduty.com/pdagent/deb/InRelease...
Downloading http://packages.pagerduty.com/pdagent/deb/Release...
Downloading http://packages.pagerduty.com/pdagent/deb/Release.gpg...
gpgv: Signature made Thu Jul 20 21:33:41 2017 UTC using RSA key ID 76203C00
gpgv: Can't check signature: public key not found
ERROR: unable to fetch mirror: verification of detached signature failed: exit status 2

But it's not my fault.

[smira]

Thanks, I'll take a look

[aol-nnov]

rebased PR on tip of master after v1.1.0 release

[hoegaarden]

Hey @aol-nnov , Hi all ...

Sorry for being so unresponsive, that PR (my original and this resurrection) totally slipped under my radar, I don't need that functionality anymore and totally forgot about it. I will try to check if any of the original comments are not addressed yet.

Bye & Thanks @aol-nnov for keeping track of that.

[sliverc]

Just had a brief look at this PR.

Do I understand this correctly that the hook script is running in parallel with the api action? Is this intended?

I think it would be better to have pre-hooks.d and post-hooks.d directories with scripts sorted by alphabet. This way several hooks can be added and a specific state of the call.

Also hooks I think should not be fired and forget but run sync and need to return exit code 0 when successful (only this way can we guarantee that the hook has actually been executed).

Also if it runs in sync the hook knows in what state the repository is as in async thing can change at any time (second request happening while hook script is still running etc.).

If the scripts still decides to run a command async it can still do so and if need to it also allows a script to stop a api request by returning a exit code != 0.

What do you think?

[hsitter]

This is all very true.

BUT... 😉

When looking at issue #388 I get the feeling that the PR is too broad in scope. To solve the original issue a fire-and-forget hook that is run exclusively after publish is done, with minimal information passed to it, would entirely be sufficient. The goal would be to notify that we published and something needs to happen now. Whether or not that something is successful doesn't matter, we notified and that's the extent of our involvement. Another publish happening while the notification is still being handled also doesn't matter (assuming AcquireByHash, which I should think is a given with mirrors).
Pretty much all the concerns @sliverc mentions wouldn't apply. It'd be more of a notification system than a hook system.

To notify a mirror system to sync we don't need a fully blown hook system.

And that leaves me wondering: Do we really need a hook system? What would that hook system be used for exactly?

With all that said I am actually content with the current PR. If I were to change something it'd be to only call the helper binary on publish and only give it the final publish directory cmd: apihook s3:foobar as information. Ultimately that is all we need to solve #388.

[sliverc]

@apachelogger Valid point. Maybe if we don't want resp. need a full blown hook system we would need to rethink how this feature it is called. In dev terms it would be more of a signal or event than a hook. But open to suggestions ;).

[hsitter]

Referring to it as signal would be better indeed.

[hoegaarden]

When I hear signal I think about OS / process signals -- I'd prefer event. But maybe that's just me ...

[hsitter]

Clearly you haven't heard of https://signal.org/ 😛

It's up to whoever makes the changes to be honest. The reason I'd not go with event is that we could totally end up having an actual event system on the API-side. (e.g. like https://api.slack.com/events-api which would basically be the API version of a hook system like outlined in a previous comment).