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

Send notifications when generic/unhandled failures #3864

Merged
merged 1 commit into from Oct 2, 2018

Conversation

@humitos
Copy link
Member

@humitos humitos commented Mar 27, 2018

This PR used to have a test case but it wasn't accurate. I decided to remove it and leave this small piece of code untested (send an email on unhandled failures). It shouldn't be terrible and it does fix an issue that our customer are having at the moment.

We can improve this later when a re-work is done around the UpdateDocsTask and how all the steps of this task are managed.

@agjohnson agjohnson added this to the 2.4 milestone Apr 11, 2018
@humitos humitos requested a review from agjohnson Apr 16, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Apr 16, 2018

@agjohnson can you take a look at this PR?

  • I'm not sure how to do a proper test for this.
  • I suppose the logic is correct regarding that case where we were missing to send the email if there is a more generic exception.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Apr 18, 2018

Hrm, yeah I'm not sure of the best pattern to use here. Moving all this logic into context managers was the original fix to centralize steps like build reporting. It seems we're just leaking more and more state out of the context managers.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Apr 25, 2018

I think if we want to accept the duplication of this logic for now, we should continue to test this and merge it. We can come back and cleanup and implement #3984 to reduce all of this duplication.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Apr 25, 2018

Regarding testing, is it possible to throw and exception as a mock side effect on one of the calls that would normally trigger this bug?

@humitos humitos force-pushed the humitos/notifications/generic-failures branch from 3472fc3 to f852fb9 May 9, 2018
@agjohnson agjohnson removed this from the 2.4 milestone May 31, 2018
@agjohnson agjohnson added this to the 2.5 milestone May 31, 2018
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 6, 2018

@humitos what is the state of this? Can it be closed, since it was just a test?

@agjohnson agjohnson removed this from the 2.5 milestone Jun 7, 2018
@agjohnson agjohnson added this to the 2.6 milestone Jun 7, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Jun 11, 2018

@ericholscher this isn't just a test. In fact, this solves a problem reported in the corporate site: when a generic exception happens an email is not sent to the user. This PR makes that email/notification to go out properly.

The problem on this PR is that I didn't find a way to write an accurate test because our update task flow exception handling depends on self. variables while handling the exception that are not being declared yet (sometimes). I will remove the test case and rebase with master leaving the FIXME comment for future reference.

@humitos humitos force-pushed the humitos/notifications/generic-failures branch 2 times, most recently from 9905696 to 2fec3a6 Jun 11, 2018
@humitos humitos requested a review from Jun 11, 2018
@agjohnson agjohnson removed this from the 2.6 milestone Jul 17, 2018
@agjohnson agjohnson added this to the 2.7 milestone Jul 17, 2018
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 27, 2018

Needs conflicts resolved.

@humitos humitos self-assigned this Oct 1, 2018
@humitos humitos force-pushed the humitos/notifications/generic-failures branch from 2fec3a6 to 41a05f7 Oct 1, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Oct 1, 2018

Conflicts solved and this should be ready to merge after tests pass.

@humitos humitos merged commit be59bbc into master Oct 2, 2018
1 check passed
@humitos humitos deleted the humitos/notifications/generic-failures branch Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants