Extend webhook notifications with build status (queued, running, failed, succeeded) & commit id

[Korijn]

Webhook build notifications currently trigger only on build failure. Also, the commit_id that was built is missing from the JSON object that is sent.

If possible, I'd like to extend this feature to trigger on any build status transition:

• queued
• running
• failed
• succeeded

Also, in order to integrate with e.g. Bitbucket/Github build status APIs, the commit_id should be included in the JSON object that is sent.

Requirements

Webhooks are not currently set up as an asynchronous task, meaning sending these notifications will block the process. We should instead be executing these as Celery tasks. The function that currently sends these notifications is here:
https://github.com/rtfd/readthedocs.org/blob/712e75d30a92bbdcf3a2597dafc16c58897eeefc/readthedocs/projects/tasks.py#L776

This will require the following work items:

• All current calls to webhook_notification should be changed to execute a task instead. This will require setting webhook_notification as a Celery task as well. This should only execute on the web task queue.
• webhook_notification should add the Build.commit attribute to the data payload
• Add asynchronous task calls to other points in the build system -- when we queue a build, when we start a build, and when we finish a build. Currently, we only call this when the build fails.
• Emails should only send on failure, webhooks should send regardless of the build status at the end of the build task

Here is the actual build task:
https://github.com/rtfd/readthedocs.org/blob/712e75d30a92bbdcf3a2597dafc16c58897eeefc/readthedocs/projects/tasks.py#L105

Here is where builds currently send failure notifications:
https://github.com/rtfd/readthedocs.org/blob/712e75d30a92bbdcf3a2597dafc16c58897eeefc/readthedocs/projects/tasks.py#L185

Builds are triggered from:
https://github.com/rtfd/readthedocs.org/blob/f6603f40599fac810993989cffc55da82f19feeb/readthedocs/core/utils/__init__.py#L67

[agjohnson]

Both of these additions sound like features we should have. I think this stands to be a single ticket here. It should be simple enough to attribute the changeset that triggered the build. In order to trigger webhook notifications on extraneous functions, we would have to add more task triggers in the project build functions. Currently, the notifications are only triggered at the point that the project build failed.

This is the function to call, however there should really be a wrapper task around this to send these additional notifications asynchronously:
https://github.com/rtfd/readthedocs.org/blob/712e75d30a92bbdcf3a2597dafc16c58897eeefc/readthedocs/projects/tasks.py#L776

[shubheksha]

@agjohnson, I'd like to take this up!

[agjohnson]

Great! If you need some more direction, I can update the description here a bit further. Let me know if you have questions on any implementation details, I'm happy to answer.

[shubheksha]

Sorry about getting back so late! It would be of immense help if you can update the description further & give me a little direction on how to start. I've been going through the celery docs to familiarize myself with tasks.

[gvanrossum]

@agjohnson Could you help @shubheksha out here? She's trying to contribute for the first time and she would like you to give her some more specific guidance on this particular issue. E.g. is reading up on celery useful at all? What exactly did you mean by "there should really be a wrapper task around this"? (I wish I knew this code base better so I could answer that myself!)

[Korijn]

@agjohnson Are you too busy? I can work out a plan for @shubheksha if you're unavailable.

[gvanrossum]

Please do help!

[agjohnson]

Sorry all, this fell off my radar :( I can expand on some of what i laid out above, but @Korijn, more help would be always be welcome here as well!

[agjohnson]

@shubheksha I updated the description with some more explicit information on how this would be implemented. Feel free to raise a question here if something doesn't make sense, I'm happy to provide more guidance.

This will require some basic knowledge of Celery, but only enough to implement a task decorator around the current webhook_notification function and turn the current calls into task calls. Currently, we're executing webhook_notification directly from the project build task.

[shubheksha]

Thank you, @Korijn & @gvanrossum. 😄
@agjohnson - thank you for the expanded description. I will try to get something working & ask for further guidance in case I get stuck!

[humitos]

@shubheksha were you able to start working on this? are you still interested on this issue? Maybe we can take a look together.

[safwanrahman]

All current calls to webhook_notification should be changed to execute a task instead. This will require setting webhook_notification as a Celery task as well. This should only execute on the web task queue.

@Korijn There are only one calls of webhook_notification is send_notifications task. As send_notifications is already a task and is called assynchronously everywhere, I think creating task for each webhook notification is overkill.
https://github.com/rtfd/readthedocs.org/blob/712e75d30a92bbdcf3a2597dafc16c58897eeefc/readthedocs/projects/tasks.py#L732-L739

I think this issue should cover sending webhook notification all the time of the build, while its queyed, failed or successed.

[73VW]

Has anyone made progresses on this?

How can I help?

I can develop this myself if you wish. I simply need someone to update the description.

Apparently the notification process is already a task so I should just have to wire things in order to have notifications on build queue/startup/finish, right?

I was thinking about making a Gitter integration for Read the Docs but I saw that there was only the failure notification. It would be really nice if we could implement this then we could have all build status appear in Gitter activity tab just like for Travis or others.

[Edit]
I can also update the doc according to this functionnality if I implement it!

[Edit 02.11.2018]
@agjohnson is this idea still alive?

[shivanshu1234]

Has this issue gone stale? If not, I would like to work on it.

[ZeyadYasser]

Seems like nobody is working on this, I will start working on it.

[ZeyadYasser]

As @safwanrahman said send_notifications is already a task and is called asynchronously, I think it would be sufficient to adjust it so it only sends emails if the build fails, and just use it for other states (queued, running, failed, succeeded).
https://github.com/rtfd/readthedocs.org/blob/e565fce80abf04f518c92b7297d0d8edcdb2de86/readthedocs/projects/tasks.py#L1352-L1365
Please correct me if I am wrong.

[Korijn]

Sounds good to me? Why don't you give it a try?

There lies an additional challenge in supporting the build status APIs of github and bitbucket and the like. They require perhaps a different format (check their docs).

[ZeyadYasser]

While working on this I faced a design issue.
Github's build status webhook url depends on the sha of the commit
e.g. https://api.github.com/repos/:owner/:repo/statuses/:sha
so the user will not be able to supply it as webhook in the settings of his project as it might vary from build to build.
I found that the case is similar with gitlab and bitbucket status APIs.
Do you think we should maybe allow placeholders? so the supplied url would be something like this maybe
https://api.github.com/repos/:owner/:repo/statuses/{commit_hash} where commit_hash will be replaced with the commit hash before sending the request.
I think that it might not be intuitive in the UI, maybe a hint in the (notifications settings) page would make it better?

[karnicaa]

Hi. Is this issue still open?

[stsewd]

@karnicaa yes, there is a PR for it #5621