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

Projects
None yet
3 participants
@humitos
Member

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

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Apr 16, 2018

Member

@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.
Member

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

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Apr 18, 2018

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Apr 25, 2018

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Apr 25, 2018

Contributor

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?

Contributor

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?

@agjohnson agjohnson modified the milestones: 2.4, 2.5 May 31, 2018

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Jun 6, 2018

Member

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

Member

ericholscher commented Jun 6, 2018

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

@agjohnson agjohnson modified the milestones: 2.5, 2.6 Jun 7, 2018

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Jun 11, 2018

Member

@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.

Member

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 requested a review from rtfd/core Jun 11, 2018

@agjohnson agjohnson modified the milestones: 2.6, 2.7 Jul 17, 2018

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Sep 27, 2018

Contributor

Needs conflicts resolved.

Contributor

agjohnson commented Sep 27, 2018

Needs conflicts resolved.

@humitos humitos self-assigned this Oct 1, 2018

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Oct 1, 2018

Member

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

Member

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

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@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