Skip to content
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

Explicitly log not sending mails if no build result is set + fix some German umlauts apart from Javadoc warnings #44

Merged
merged 24 commits into from
Mar 29, 2019

Conversation

ottlinger
Copy link
Contributor

While running into
https://stackoverflow.com/questions/37169100/use-jenkins-mailer-inside-pipeline-workflow
I realized that an explicit logging is needed if no build result is set.

During pipeline runs no result is set and no mail is sent out.

@ottlinger ottlinger changed the title Explicitly handle not sending mails of no build result is set Explicitly handle not sending mails if no build result is set Oct 12, 2018
Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I see what you've changed here.

}

System.out.println("Not sending any mail as BuildResult is not set at all.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use a logger here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the "standard" way for logging in Jenkins plugins? Log4j? Slf4J? Therefore I thought that StandardOut-Logging would result in an output in the job console .....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard way is either directly through java.util.logging or via SLF4J.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if java.util.logging works, but replaced the line in the code - thanks.

@ottlinger
Copy link
Contributor Author

I ran into the problem that no mails were sent from a Pipeline build as the currentBuild.result was set to null. To make this state more explicit I would like the plugin to log something.

While trying to get the builds green; i stumbled upon some Javadoc issues and tried to fixed them.

Sorry for not providing more context in the first place.

@ottlinger ottlinger changed the title Explicitly handle not sending mails if no build result is set Explicitly log not sending mails if no build result is set and fix some German umlauts apart from Javadoc warnings Oct 12, 2018
Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. The reason you're not seeing any logger output is because you need to enable logging at INFO or lower for the specified logger. You can configure that via the logging properties file or through the GUI.

@oleg-nenashev
Copy link
Member

Thanks @ottlinger! looks good, will merge it

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO build log should be used instead of the system log in the warning message

src/main/java/hudson/tasks/MailSender.java Outdated Show resolved Hide resolved
@oleg-nenashev
Copy link
Member

Sorry, discovered the issue only during the second iteration

@ottlinger
Copy link
Contributor Author

@oleg-nenashev do you mean?
listener.getLogger().println();
instead of the logger I introduced.

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Oct 21, 2018 via email

-Organized imports after removing newly introduced logger.
@ottlinger
Copy link
Contributor Author

@oleg-nenashev thanks for making the change clearer to me. As I'm not that much into Jenkins plugins I need a little bit more help :-)

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Oct 21, 2018 via email

@ottlinger
Copy link
Contributor Author

np - it gave me a chance to learn more.

@ottlinger
Copy link
Contributor Author

@oleg-nenashev are you ok with the current changes? Can you start the merge? Thanks.

Copy link
Member

@alecharp alecharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the defaultGoal and add the comments in the javadoc field? Thanks.

pom.xml Outdated Show resolved Hide resolved
src/main/java/hudson/tasks/MailAddressResolver.java Outdated Show resolved Hide resolved
src/main/java/hudson/tasks/MailAddressResolver.java Outdated Show resolved Hide resolved
src/main/java/hudson/tasks/MailSender.java Outdated Show resolved Hide resolved
@ottlinger
Copy link
Contributor Author

@oleg-nenashev @alecharp @jvz
Hopefully I fixed all of the javadoc warnings so far - looking forward to your review comments

@alecharp
Copy link
Member

@ottlinger the hpi:custom-war is different. It will create a war file, which would be Jenkins+plugins. hpi:run start a Jenkins instance locally. Please change the defaultGoal to hpi:run or remove it completely. Thank you for your patience with me.

You may generate a locally running instance with
mvn hpi:custom-war hpi:run
Simplified extraction of changes from line break logic.
(Linebreak does not seem to work properly, but will file a different issue for that)
@ottlinger
Copy link
Contributor Author

Merged upstream master into my PR to make it ready for merge - pls start your reviews @oleg-nenashev @alecharp @jvz

@ottlinger
Copy link
Contributor Author

ping

@ottlinger
Copy link
Contributor Author

@alecharp @oleg-nenashev can you recheck the PR and merge. I just updated to the latest changes in master. (I'd be happy to get the merges into the next release).

@ottlinger
Copy link
Contributor Author

ping @jvz @alecharp @oleg-nenashev - any chance this PR can get merged? Thanks.

@jvz
Copy link
Member

jvz commented Feb 8, 2019

I'm not able to merge (no permissions). Oleg or Adrien might need to do it.

@alecharp
Copy link
Member

Sorry for the delay. To me the code seems fine, even if you change some imports ordering, which would have been great not to have but 🤷‍♂️

The last bit that trouble me is still the defaultGoal but it's not something major.

I'd like to give @oleg-nenashev some time to say if the modification he asked for are meeting his requirement. When that's done, I'll merge and release.

Again thank you @ottlinger for the contribution.

@ottlinger
Copy link
Contributor Author

ping @oleg-nenashev - are there any other with write access to this repo that could merge this PR? Thanks.

@ottlinger
Copy link
Contributor Author

ping @oleg-nenashev - anyone else with write access? Thanks for your help.

@oleg-nenashev
Copy link
Member

I believe @alecharp is the current maintainer

@oleg-nenashev
Copy link
Member

I can ship it since Adrien approved it

@ottlinger
Copy link
Contributor Author

@alecharp can you take a look after @oleg-nenashev s comment?! Thanks 👍

@ottlinger
Copy link
Contributor Author

@alecharp can you do the merge - @oleg-nenashev thanks.

@alecharp alecharp merged commit 6b8ea83 into jenkinsci:master Mar 29, 2019
@alecharp
Copy link
Member

Sorry, I missed your previous notification @ottlinger. Thank you for your contribution.

@ottlinger
Copy link
Contributor Author

Thanks for your help @alecharp & @oleg-nenashev

@oleg-nenashev oleg-nenashev changed the title Explicitly log not sending mails if no build result is set and fix some German umlauts apart from Javadoc warnings Explicitly log not sending mails if no build result is set + fix some German umlauts apart from Javadoc warnings Jul 26, 2019
@jglick jglick mentioned this pull request Dec 28, 2021
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.

4 participants