Skip to content
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

Extend webhook notifications with build status #5621

Merged
merged 4 commits into from Dec 9, 2019

Conversation

@saadmk11
Copy link
Member

saadmk11 commented Apr 22, 2019

I did not add support for build status API of GitHub, Gitlab etc. as those need a different approach than what we currently have. We need to send the sha of the commit with the URL as the their docs Suggests.
ie: POST /repos/:owner/:repo/statuses/:sha
Github Build Status API Docs Gitlab also has a similar implementation.
We need some Design Decisions for adding support for this. We can add some kind of module to handle the status APIs.

We are sending PR Build Status reports using GitHub Status API #5865 and planning to extend to other providers.

closes #2251

@stale

This comment has been minimized.

Copy link

stale bot commented Jun 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale label Jun 6, 2019
@ericholscher ericholscher requested a review from readthedocs/core Jun 6, 2019
@stale stale bot removed the Status: stale label Jun 6, 2019
@ericholscher

This comment has been minimized.

Copy link
Member

ericholscher commented Jun 6, 2019

We should look at this. It seems useful :) Sorry it's been sitting.

Copy link
Member

ericholscher left a comment

This is a neat concept. It likely needs a bit more work and definitely some more docs for users so they know they are now getting many more webhooks.

@@ -1353,19 +1366,20 @@ def _manage_imported_files(version, path, commit):


@app.task(queue='web')
def send_notifications(version_pk, build_pk):
def send_notifications(version_pk, build_pk, send_email=True):

This comment has been minimized.

Copy link
@ericholscher

ericholscher Jun 6, 2019

Member

This should probably default to False, so that we don't accidentally send users email. Then we can just update the places where we do want to send email.

@@ -1440,6 +1454,8 @@ def webhook_notification(version, build, hook_url):
'slug': project.slug,
'build': {
'id': build.id,
'commit': build.commit,
'state': build.state,

This comment has been minimized.

Copy link
@ericholscher

ericholscher Jun 6, 2019

Member

We should define what data we send here in the docs as part of this PR, it will be much more useful to users that way :)

@saadmk11

This comment has been minimized.

Copy link
Member Author

saadmk11 commented Jun 6, 2019

@ericholscher this needs a bit of work and polishing. We will use the Github Status API to send notifications also as a part of the PR Builder. So, I think we should start working on this again when we get to that point to implement it in a better way if possible.

@saadmk11 saadmk11 closed this Aug 7, 2019
@saadmk11 saadmk11 force-pushed the saadmk11:extend-webhook-notification branch from 45f6945 to 6767b16 Aug 7, 2019
saadmk11 added 3 commits Aug 7, 2019
@saadmk11

This comment has been minimized.

Copy link
Member Author

saadmk11 commented Aug 7, 2019

Now we can send webhook notifications when the build has triggered and when the build has finished.

{
     "name": "Read the Docs",
     "slug": "rtd",
     "build": {
         "id": 6321373,
         "commit": "e8dd17a3f1627dd206d721e4be08ae6766fda40",
         "state": "finished",
         "success": false,
         "date": "2017-02-15 20:35:54"
     }
}

But One issue is when the build has triggered we don't have the build.commit on the build object as we don't pass it for internal versions. so, it is set to null on the webhook data.

@saadmk11 saadmk11 requested review from readthedocs/core and ericholscher Aug 7, 2019
@saadmk11

This comment has been minimized.

Copy link
Member Author

saadmk11 commented Aug 7, 2019

I don't think this is blocked anymore so we can remove blocked label. :)

@saadmk11

This comment has been minimized.

Copy link
Member Author

saadmk11 commented Sep 1, 2019

Removing blocked as this is not blocked anymore

@stale

This comment has been minimized.

Copy link

stale bot commented Oct 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Copy link
Member

humitos left a comment

I think this is useful.

There is one thing that I'm not sure how to handle. We were sending a webhook notification only when the builds failed. So, people were taking an action in that case --now we are going to start sending webhook notification on start and finish (success and fail). This could cause a lot of problems if people is only expecting to receive the webhook on failures.

I think if we want to make this change to happen, we need a way to keep the current behavior and add something new for the Hook objects where the user can decide what kind of webhooks wants to receive.

docs/guides/build-notifications.rst Show resolved Hide resolved
readthedocs/projects/tasks.py Show resolved Hide resolved
@saadmk11

This comment has been minimized.

Copy link
Member Author

saadmk11 commented Nov 19, 2019

There is one thing that I'm not sure how to handle. We were sending a webhook notification only when the builds failed. So, people were taking an action in that case --now we are going to start sending webhook notification on start and finish (success and fail). This could cause a lot of problems if people is only expecting to receive the webhook on failures.

I think if we want to make this change to happen, we need a way to keep the current behavior and add something new for the Hook objects where the user can decide what kind of webhooks wants to receive.

Thanks for the review! 👍

Users will get the usual notifications as before but it will include some extra ones. Not sure if that will break things :). If we want we can add some kind of option to let users select this. Something like check boxes with

  • Get Failure Notification
  • Get Success Notification
  • Get Build Start Notification
@humitos

This comment has been minimized.

Copy link
Member

humitos commented Nov 19, 2019

Users will get the usual notifications as before but it will include some extra ones. Not sure if that will break things :)

If we are sending the notifications as before, I'm 👍 with this PR. I wasn't aware that we were sending webhook notification on trigger, success and failed. Thanks for clarifying this.

There is no need to manually select what status you want to receive notifications. I wanted that only if we were changing the behavior.

@saadmk11

This comment has been minimized.

Copy link
Member Author

saadmk11 commented Nov 20, 2019

If we are sending the notifications as before, I'm with this PR. I wasn't aware that we were sending webhook notification on trigger, success and failed. Thanks for clarifying this.

@humitos Actually I meant to say that we were sending webhook notifications on failure but this PR will include success and build start webhook notification with it.

@humitos humitos merged commit 1de81ca into readthedocs:master Dec 9, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@saadmk11 saadmk11 deleted the saadmk11:extend-webhook-notification branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.