[JENKINS-27262]: email is not sent

[fhuberts]

See the issue at https://issues.jenkins-ci.org/browse/JENKINS-27262

[jenkinsadmin]

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

[fhuberts]

ping!

[olivergondza]

It lacks test for sure, I am not sure bout the surrounding code coverage...

[fhuberts]

So now what?

In my defense: there were also no tests for the commit that broke this...

[daniel-beck]

@fhuberts It would be great if you could cover this part of the code with some tests. This will also prevent a future change from just breaking your use case if it's covered by a test.

[oleg-nenashev]

It worth to print a warning that e-mail resolution is falling back to the user ID. In such case it worths to try the "MailAddressResolver" extension point in Jenkins core.

[fhuberts]

There was no warning before so why should there be one now?

[oleg-nenashev]

@fhuberts
Before the change you would get Messages.MailSender_NoAddress warning for this case

[fhuberts]

Might be. But before the change that broke it there was no warning.

And in fact I think that falling back is actually wrong.
In fact I want NO resolving of the email address.
I have a mail server setup that does that for me.
So should everyone else. Mail with a wrong address will get bounced and delivered to the postmaster.

Resolving against jenkins users is wrong. Might be convenient for some people, but wrong anyway.

[oleg-nenashev]

But before the change that broke it there was no warning.

Now we have an opportunity to improve the code. Just use a convenient logging level (INFO or FINE)

Resolving against jenkins users is wrong. Might be convenient for some people, but wrong anyway.

This is a right way in Jenkins, because there are many ways to determine it in Jenkins including e-mails explicitly set by users on their profile pages. BTW the code above your check already calls Mail address resolvers, so it should work for registered users.

For non-registered users the common approach is to add e-mail suffixes if they're specified. If you don't specify the e-mail suffix from the global config, you will get the behaviour you want => it's a convenience way.

[fhuberts]

Well, my Jenkins servers are tightly coupled to my git server and the jenkins server has a local MTA that has the MTA of the git server as a relayhost.
This is not an uncommon setup.

I specify simple (git server) mail aliases in jenkins jobs.
Again, not uncommon.

I can understand that resolving against jenkins users is needed for some fields, but for email? Come on. Job reports are not normally emailed to individual users are they? They go to groups and lists.

Anyway, we can argue all we want but it doesn't seem useful.

Can I expect progress on merging this patch?
It only adds a fallback and should therefore not break anything out there.

[oleg-nenashev]

I can understand that resolving against jenkins users is needed for some fields, but for email? Come on.

This is a main functionality of the Mailer plugin. If you disagree with it, you can consider creating a plugin for your use-cases. Adding extra behavior configs to Mailer can be considered as well.

Can I expect progress on merging this patch?

You've been asked to provide unit tests and a logging of the case. If you address this comments, you will get a +1 at least from me

[fhuberts]

So why the higher standard for me?
There are no tests now

[fhuberts]

Someone more familiar with the code can write these tests way faster than me and I have no interest in becoming a contributor.
This is a drive-by fix

[oleg-nenashev]

It's a problem with the original code, but it's not a standard. It's just an issue to be fixed. BTW some tests are available in Jenkins harness and acceptance test suites.

Someone more familiar with the code can write these tests

If somebody agrees with this approach, then the PR gets merged. Since there's no active maintainer of the plugin, we need a joint agreement. I suppose somebody from Jenkins contributors will consider finalizing this PR when he needs a new release, but open-source contributors don't provide any SLAs.

For the current PR state I'd abstain. Probably @daniel-beck or @olivergondza are ready to accept the PR.

[daniel-beck]

try the "MailAddressResolver" extension point in Jenkins core.

@oleg-nenashev FYI It's now in this plugin.

[daniel-beck]

@fhuberts Wouldn't a default email suffix of @localhost or similar in Configure Jenkins work here? Have you tried that?