Skip to content

feature: email notification for all stages of a pipeline.#229

Closed
phoenix24 wants to merge 5 commits intogocd:masterfrom
ashwanthkumar:feature-notification-all-stages
Closed

feature: email notification for all stages of a pipeline.#229
phoenix24 wants to merge 5 commits intogocd:masterfrom
ashwanthkumar:feature-notification-all-stages

Conversation

@phoenix24
Copy link
Contributor

This pull request allows users to add email notifications for all stages of any given pipeline.
At present one needs add notification or every stage of a given pipeline, this pull request eases this by allowing user to add a notification on all-stages of any pipeline.

It tries to do this by introducing a new label called [ALL STAGES], returning it as part of the stages list when viewed via MyController. We've also updated the test cases and found the feature changes to work as expected to when tested locally.

The choice for label "[ALL STAGES]" made sense to use, because its descriptive enough for the purpose and users are prohibited to create any pipelines or stages with starting with special characters.

@phoenix24 phoenix24 changed the title feature: email notification all stages feature: email notification for all stages of a pipeline. Jun 3, 2014
@kmugrage
Copy link
Contributor

kmugrage commented Jun 3, 2014

I wonder if the label should be "[ANY STAGE]". To me, ALL implies that they all have to pass or fail to be notified.

@phoenix24
Copy link
Contributor Author

@kmugrage Yeah the label "[ANY STAGE]" sounds a better descriptor for the functionality.
If this sounds good to everyone, I'd go ahead and make the necessary changes.

Please Let me know.

@arvindsv
Copy link
Member

@phoenix24 Let's wait a day or two to see if anyone responds. If not, I think you can make that change.

@sriramnrn
Copy link

+1 from me for "[ANY STAGE]".

-- Ram

On Thu, Jun 12, 2014 at 9:57 AM, Aravind SV notifications@github.com
wrote:

@phoenix24 https://github.com/phoenix24 Let's wait a day or two to see
if anyone responds. If not, I think you can make that change.


Reply to this email directly or view it on GitHub
#229 (comment).


Belenix: www.belenix.org
Twitter: @sriramnrn

@srinivasupadhya
Copy link
Contributor

add to the confusion: how about [EVERY STAGE] :)

@kmugrage
Copy link
Contributor

To me, every means that all 4 stages of a 4 stage pipeline would have to
fail.
On Jun 11, 2014 9:34 PM, "srinivas upadhya" notifications@github.com
wrote:

add to the confusion: how about [EVERY STAGE] :)


Reply to this email directly or view it on GitHub
#229 (comment).

@kmugrage
Copy link
Contributor

@srinivasupadhya are you ok with [ANY STAGE] so this can be changed and merged?

@srinivasupadhya
Copy link
Contributor

fine with it! :)

@arvindsv
Copy link
Member

Since this looks like it'll get merged soon, I thought I'd take a look at it. You've removed a call to hasSubscribedFor(). I think this will end up making every user (who has permission to see that pipeline) get a notification on failure (not checking whether they subscribed for that notification or not).

The test that fails for this, is UserServiceIntegrationTest#shouldLoadUsersWhoSubscribedNotificationOnStage.

Also, this pull request has commits from other pull requests too. Can you please fix that too?

@mdaliejaz mdaliejaz added this to the Release 14.2 milestone Jun 19, 2014
@arikagoyal arikagoyal removed this from the Release 14.2 milestone Jun 19, 2014
@arvindsv
Copy link
Member

Closing this for now. Has seen no activity for more than a month. Can be fixed and reopened later, I suppose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants