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

Open to PR with Additional Logging? #508

Closed
sghill opened this issue Feb 7, 2019 · 2 comments
Closed

Open to PR with Additional Logging? #508

sghill opened this issue Feb 7, 2019 · 2 comments

Comments

@sghill
Copy link
Contributor

sghill commented Feb 7, 2019

I'd like to add additional logging to this plugin to gain some insight around the decisions it's making to send notifications. I'm happy to submit a PR, but first I thought I'd ask: is that likely to be accepted?

For example, the logic in ActiveNotifier.java#L123 is quite complex. Occasionally we'll troubleshoot something like notifications not coming through for multiple failed builds in a row, only to realize the advanced setting 'Notify on Repeated Failure' was not checked. If there were logs we could look to when a question arose, it'd really shorten the troubleshooting process.

There are places where logs exist that I'd like to be more detailed as well. For example ActiveNotifier.java#L182 has

logger.info("No change set computed...");

but this is in the context of a build, and it'd be really helpful to have that information present when troubleshooting a particular job:

logger.info("No change set computed for $projectName#$buildNumber");
@jetersen
Copy link
Member

jetersen commented Feb 7, 2019

Again I think office365 plugin does it best or at least tries to 😄
We avoided the while loop so that's an improvement.
https://github.com/jenkinsci/office-365-connector-plugin/blob/master/src/main/java/jenkins/plugins/office365connector/Office365ConnectorWebhookNotifier.java#L152

This could use a rewrite to use the parent class Run instead of AbstractBuild

String getChanges(AbstractBuild r, boolean includeCustomMessage) {

@timja
Copy link
Member

timja commented Feb 7, 2019

I'm scared to touch that code (also I don't use that functionality so I'm less likely to notice if something goes wrong there) but a PR would certainly be accepted that refactors it

@Casz's example is much better code than the state this part of the code currently is in

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

No branches or pull requests

3 participants