Skip to content

Remove notify phase#1638

Merged
gazpachoking merged 1 commit intodevelopfrom
remove_notify_phase
Jan 23, 2017
Merged

Remove notify phase#1638
gazpachoking merged 1 commit intodevelopfrom
remove_notify_phase

Conversation

@gazpachoking
Copy link
Copy Markdown
Member

Motivation for changes:

Notify phase was added to enable plugins that don't end up propagating errors to task. We since combined all notify plugins into one master 'notify' plugin, which means there is already a common place to catch the errors without a special phase.

Detailed changes:

  • Remove notify phase. 'notify' plugin runs on output phase and catches child plugin errors itself

Addressed issues:

@paranoidi
Copy link
Copy Markdown
Member

Much improved, +1

@liiight
Copy link
Copy Markdown
Member

liiight commented Jan 23, 2017

I think there's a potential issue. What if atask is defined under entriesand task has several reruns? The user will get a notification per task run right?

@gazpachoking
Copy link
Copy Markdown
Member Author

I think there's a potential issue. What if a task is defined under entries and task has several reruns? The user will get a notification per task run right?

Not quite sure what you mean by "a task is defined under entries", but yes, task notifications will be sent once per rerun. I don't think that behavior has changed though, and don't really see a way around it.

@liiight
Copy link
Copy Markdown
Member

liiight commented Jan 23, 2017

Sorry I meant task configured under notify, you got the idea.
Can we pass stuff under notify > task to run on exit phase?

@gazpachoking
Copy link
Copy Markdown
Member Author

Can we pass stuff under notify > task to run on exit phase?

Sure, but that you'll only get notified about entries in the last rerun of the task, which I think is even less desirable than multiple notifications.

@gazpachoking
Copy link
Copy Markdown
Member Author

To do proper combined emails you sorta need a digesting task (at the moment.)

@liiight
Copy link
Copy Markdown
Member

liiight commented Jan 23, 2017

Well I guess it's out of the scope of this, but aggregating entries in different reruns would be nice. I see no other reason not to merge this then.

@gazpachoking gazpachoking merged commit bec3fa6 into develop Jan 23, 2017
@cvium cvium deleted the remove_notify_phase branch June 5, 2017 14:55
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.

3 participants