adding "Emergency reroute" option to email-ext-plugin #20

Merged
merged 14 commits into from Jan 25, 2012

Projects

None yet

4 participants

@hhuynh
Contributor
hhuynh commented Oct 12, 2011

Our Jenkins cluster requires some maintenance work during which we
prefer all Jenkins emails (failures, success, unstable, etc) to be
rerouted to an email address (or addresses).

I've added this option in my forked git repo of email-ext-plugin.
There's now an extra "Emergency reroute" in the plugin configuration
page. If it's not empty, then that will be the recipient list of the
current email excluding all other potential recipients.

@slide slide and 1 other commented on an outdated diff Oct 14, 2011
...lugins/emailext/ExtendedEmailPublisherDescriptor.java
@@ -328,6 +338,9 @@ public class ExtendedEmailPublisherDescriptor extends BuildStepDescriptor<Publis
defaultBody = nullify(req.getParameter("ext_mailer_default_body"));
recipientList = nullify(req.getParameter("ext_mailer_default_recipients")) != null ?
req.getParameter("ext_mailer_default_recipients") : "";
+
+ emergencyReroute = nullify(req.getParameter("ext_mailer_emergency_reroute")) != null ?
@slide
slide Oct 14, 2011 Member

I think you may want to just have the nullify call like defaultBody and not follow the recipientList example.

@hhuynh
hhuynh Oct 14, 2011 Contributor

Thanks for the tip. I've pushed that change.

Hung-

On Fri, Oct 14, 2011 at 10:13 AM, slide <
reply@reply.github.com>wrote:

@@ -328,6 +338,9 @@ public class ExtendedEmailPublisherDescriptor extends
BuildStepDescriptor<Publis
defaultBody =
nullify(req.getParameter("ext_mailer_default_body"));
recipientList =
nullify(req.getParameter("ext_mailer_default_recipients")) != null ?
req.getParameter("ext_mailer_default_recipients") : "";
+

  •    emergencyReroute =
    
    nullify(req.getParameter("ext_mailer_emergency_reroute")) != null ?

I think you may want to just have the nullify call like defaultBody and not
follow the recipientList example.

Reply to this email directly or view it on GitHub:
https://github.com/jenkinsci/email-ext-plugin/pull/20/files#r171828

@slide
Member
slide commented Oct 27, 2011

I think this looks ok. Would like someone else to merge the pull request so there are two people who review it.

@slide
Member
slide commented Nov 18, 2011

Please rebase to master and then push again to get the latest so this can be automerged.

@hhuynh
Contributor
hhuynh commented Nov 18, 2011

Hello,

I'm a newbie to git (never used it until I needed it to change
email-ext-plugin) so could you help me with the exact commands of what
needed? (or maybe a reference page)

Thank you,

Hung-

On Fri, Nov 18, 2011 at 4:48 AM, slide <
reply@reply.github.com

wrote:

Please rebase to master and then push again to get the latest so this can
be automerged.


Reply to this email directly or view it on GitHub:
#20 (comment)

@slide
Member
slide commented Nov 18, 2011

https://wiki.jenkins-ci.org/display/JENKINS/Dev+Good+Practice
On Nov 18, 2011 11:46 AM, "Hung Huynh" <
reply@reply.github.com>
wrote:

Hello,

I'm a newbie to git (never used it until I needed it to change
email-ext-plugin) so could you help me with the exact commands of what
needed? (or maybe a reference page)

Thank you,

Hung-

On Fri, Nov 18, 2011 at 4:48 AM, slide <
reply@reply.github.com

wrote:

Please rebase to master and then push again to get the latest so this can
be automerged.


Reply to this email directly or view it on GitHub:

#20 (comment)


Reply to this email directly or view it on GitHub:
#20 (comment)

@kutzi
Member
kutzi commented Nov 18, 2011

I'm not a friend of adding too many options for all possible rare use cases.
hhuynh can you explain further what's the motivation behind this change and why you cannot achieve it in other ways - e.g. changing the smtp server address in the global config to a 'dummy' mail server?

If on the other hand, there's really the need to have an 'emergency e-mail address', shouldn't we better add it to the core config?

@hhuynh
Contributor
hhuynh commented Nov 18, 2011

During a release cycle, we delete old SNAPSHOT artifacts in Nexus and
update our poms to the next SNAPSHOT so we rely on Jenkins artifact
publishers jobs to tell us which project is failing to compile because the
old SNAPSHOTs were deleted. There are 600+ jobs in our cluster and the mail
bomb during this time annoy a lot of developers and mailing lists.

So we want to reroute emails to a group of people who working on the poms
only. This has worked great for us since we added this feature.

Another unintended benefits is that when a lot of jobs starting to fail due
to some external failures (SVN mirror down, shared network drive down, etc)
we could temporarily reroute email to Jenkins admins and work out the fix
without having to stop Jenkins master altogether.

So changing smtp to a dummy server isn't too great solution since we still
want to see whatever we're fixing has actually worked base on the
success/failure emails.

I hope it's not a self serving feature. Though I can only guess other
people might find it useful.

Thanks,

Hung-

On Fri, Nov 18, 2011 at 11:26 AM, Christoph Kutzinski <
reply@reply.github.com

wrote:

I'm not a friend of adding too many options for all possible rare use
cases.
hhuynh can you explain further what's the motivation behind this change
and why you cannot achieve it in other ways - e.g. changing the smtp server
address in the global config to a 'dummy' mail server?

If on the other hand, there's really the need to have an 'emergency e-mail
address', shouldn't we better add to the core config?


Reply to this email directly or view it on GitHub:
#20 (comment)

@kutzi
Member
kutzi commented Nov 20, 2011

Thanks for explaining. Seems reasonable to me.

unknown added some commits Nov 22, 2011
unknown rebase 79906a3
unknown rebase 887801c
unknown Merge branch 'master' of github.com:hhuynh/email-ext-plugin
Conflicts:
	src/main/java/hudson/plugins/emailext/ExtendedEmailPublisher.java
	src/main/java/hudson/plugins/emailext/ExtendedEmailPublisherDescriptor.java
711d675
@hhuynh
Contributor
hhuynh commented Nov 22, 2011

I've done the rebase. There was a conflict and I tried to resolve it. I'm
not sure I did it right though.

If you can initiate automerge now, that would be great. If it doesn't work
out, I'll try again, maybe forking from master again then reapply my change.

On Fri, Nov 18, 2011 at 4:48 AM, slide <
reply@reply.github.com

wrote:

Please rebase to master and then push again to get the latest so this can
be automerged.


Reply to this email directly or view it on GitHub:
#20 (comment)

@slide
Member
slide commented Nov 22, 2011

There is a simple reference for dev good practices on the wiki, just double
check there to see if you followed the steps.
On Nov 21, 2011 7:06 PM, "Hung Huynh" <
reply@reply.github.com>
wrote:

I've done the rebase. There was a conflict and I tried to resolve it. I'm
not sure I did it right though.

If you can initiate automerge now, that would be great. If it doesn't work
out, I'll try again, maybe forking from master again then reapply my
change.

On Fri, Nov 18, 2011 at 4:48 AM, slide <
reply@reply.github.com

wrote:

Please rebase to master and then push again to get the latest so this can
be automerged.


Reply to this email directly or view it on GitHub:

#20 (comment)


Reply to this email directly or view it on GitHub:
#20 (comment)

@hhuynh
Contributor
hhuynh commented Nov 22, 2011

Yeah, I followed those steps actually. Would be lost without it.

On Mon, Nov 21, 2011 at 6:11 PM, slide <
reply@reply.github.com

wrote:

There is a simple reference for dev good practices on the wiki, just double
check there to see if you followed the steps.
On Nov 21, 2011 7:06 PM, "Hung Huynh" <
reply@reply.github.com>
wrote:

I've done the rebase. There was a conflict and I tried to resolve it. I'm
not sure I did it right though.

If you can initiate automerge now, that would be great. If it doesn't
work
out, I'll try again, maybe forking from master again then reapply my
change.

On Fri, Nov 18, 2011 at 4:48 AM, slide <
reply@reply.github.com

wrote:

Please rebase to master and then push again to get the latest so this
can
be automerged.


Reply to this email directly or view it on GitHub:

#20 (comment)


Reply to this email directly or view it on GitHub:

#20 (comment)


Reply to this email directly or view it on GitHub:
#20 (comment)

@slide slide and 1 other commented on an outdated diff Nov 22, 2011
...lugins/emailext/ExtendedEmailPublisherDescriptor.java
@@ -85,6 +86,16 @@ public class ExtendedEmailPublisherDescriptor extends BuildStepDescriptor<Publis
private String defaultBody;
/**
+ * This is a global default recipient list for sending emails.
+ */
+ private String recipientList = "";
@slide
slide Nov 22, 2011 Member

I don't think that recipientList should be here anymore.

@hhuynh
hhuynh Nov 22, 2011 Contributor

I removed that as part of my conflict resolving. I don't see it in my local
check out.

On Mon, Nov 21, 2011 at 7:24 PM, slide <
reply@reply.github.com

wrote:

@@ -85,6 +86,16 @@ public class ExtendedEmailPublisherDescriptor extends
BuildStepDescriptor<Publis
private String defaultBody;

 /**
  • \* This is a global default recipient list for sending emails.
    
  • */
    
  • private String recipientList = "";

I don't think that recipientList should be here anymore.


Reply to this email directly or view it on GitHub:
https://github.com/jenkinsci/email-ext-plugin/pull/20/files#r244047

@hhuynh
hhuynh Nov 22, 2011 Contributor

I've deleted my local clone and recloned. Then I could see those extra
recipientList lines. I've fixed them up now.

On Mon, Nov 21, 2011 at 9:19 PM, Hung Huynh hhuynh@gmail.com wrote:

I removed that as part of my conflict resolving. I don't see it in my
local check out.

On Mon, Nov 21, 2011 at 7:24 PM, slide <
reply@reply.github.com

wrote:

@@ -85,6 +86,16 @@ public class ExtendedEmailPublisherDescriptor
extends BuildStepDescriptor<Publis
private String defaultBody;

 /**
  • \* This is a global default recipient list for sending emails.
    
  • */
    
  • private String recipientList = "";

I don't think that recipientList should be here anymore.


Reply to this email directly or view it on GitHub:
https://github.com/jenkinsci/email-ext-plugin/pull/20/files#r244047

@slide
Member
slide commented Nov 22, 2011

I think this looks reasonable, any objections to merging?

@hhuynh
Contributor
hhuynh commented Nov 28, 2011

It looks good to go for me.

On Tue, Nov 22, 2011 at 3:18 AM, slide <
reply@reply.github.com

wrote:

I think this looks reasonable, any objections to merging?


Reply to this email directly or view it on GitHub:
#20 (comment)

@slide
Member
slide commented Nov 28, 2011

kutzi any issues with merging this?

@kutzi
Member
kutzi commented Dec 1, 2011

I still wish it would be doable without a new option, but looks like there isn't. I'm not a maintainer of the plugin anyway, so I've no right to veto ;)

@hhuynh
Contributor
hhuynh commented Dec 15, 2011

could we get some love here? :)

It's my first contribute ever to Jenkins.

Thanks,

Hung-

On Thu, Dec 1, 2011 at 12:56 AM, Christoph Kutzinski <
reply@reply.github.com

wrote:

I still wish it would be doable without a new option, but looks like there
isn't. I'm not a maintainer of the plugin anyway, so I've no right to veto
;)


Reply to this email directly or view it on GitHub:
#20 (comment)

@hhuynh
Contributor
hhuynh commented Jan 25, 2012

Hi guys,

Just want to check in with this pull request. Are we good to go or is it a "NAY"? :)

Thanks,

Hung-

@slide slide merged commit 29e6bed into jenkinsci:master Jan 25, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment