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

Protection against None when sending notifications #6610

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jan 29, 2020

I remember we put this logic outside the task, so we don't create additional tasks. But we also refactor the send_notifications method to receive a pk instead of using self.version because if the API fails, it will be None.

ref https://sentry.io/organizations/read-the-docs/issues/1425845969/?project=148442&query=is%3Aunresolved

@stale
Copy link

stale bot commented Mar 14, 2020

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 Issue will be considered inactive soon label Mar 14, 2020
@stale stale bot closed this Mar 21, 2020
@stsewd stsewd removed the Status: stale Issue will be considered inactive soon label Mar 23, 2020
@stsewd stsewd reopened this Mar 23, 2020
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think sending emails on PR builds makes sense, but I can understand it might be spammy. I think we likely need to make things a bit more configurable.

I'm 👍 on this with an update to our docs explaining when we send which notifications.

@stsewd stsewd merged commit 1dc777b into readthedocs:master Apr 20, 2020
@stsewd stsewd deleted the protection-for-none-when-sending-notifications branch April 20, 2020 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants