Mention Committers On Build Failure#418
Conversation
|
|
||
| public static String mentionUser(User user) { | ||
| if (user == null) { | ||
| return "Unknown User"; |
There was a problem hiding this comment.
Unknown User
or
Unknown user ?
| if (user == null) { | ||
| return "Unknown User"; | ||
| } | ||
| Mailer.UserProperty prop = user.getProperty(Mailer.UserProperty.class); |
| return "<at>" + email + "</at>"; | ||
| } | ||
|
|
||
| public static String mentionUser(User user) { |
There was a problem hiding this comment.
name of this method does not report your real intention but rather fallback scenario - I believe this is still mentionEmail
|
|
||
| boolean isRepeatedFailure = isRepeatedFailure(previousResult, lastNotFailedBuild); | ||
|
|
||
| boolean shouldMention = (lastResult == Result.FAILURE || lastResult == Result.UNSTABLE || isRepeatedFailure) && mentionOnFailure; |
There was a problem hiding this comment.
if the current build is SUCCESS but the previous was FAILURE you are mentioning - was it your intention or you just want to provide more details only on failure?
| <f:checkbox/> | ||
| </f:entry> | ||
|
|
||
| <f:entry title="Only Mention On Failure" field="mentionOnFailure"> |
There was a problem hiding this comment.
Mention only when build has failed
| String joinedCommitters = authors.stream() | ||
| .map(User::getFullName) | ||
| .collect(Collectors.joining(", ")); | ||
| .map(user -> mentionCommitters ? TeamsMentionUtils.mentionUser(user) : user.getFullName()) |
There was a problem hiding this comment.
it complains here because you haven't extended examples in JSON file - same as for previous PR, please add it
| factsBuilder.addRemarks(); | ||
| factsBuilder.addCommitters(); | ||
| factsBuilder.addDevelopers(); | ||
| factsBuilder.addCommitters(mentionCommitters && shouldMention); |
There was a problem hiding this comment.
is it correct configuration if you set mentionCommiters to true and shouldMention to false?
There was a problem hiding this comment.
Yes — committers are always displayed. The boolean only controls whether they’re mentioned. I’ll add clarification / rename to make that intent explicit. If I were to get rid of the mention developers toggle like I talk about here #418 (comment), then I wouldn't need a mentionCommitters toggle either as those would be the only people I'd mention. The only toggle would be shouldMention so that line would turn into
`factsBuilder.addCommitters(shouldMention);`
| for (ChangeLogSet<ChangeLogSet.Entry> set : changeSets) { | ||
| for (ChangeLogSet.Entry entry : set) { | ||
| User author = entry.getAuthor(); | ||
| if (author != null) { |
There was a problem hiding this comment.
this null checking is examined twice: here and above
| this.run = run; | ||
| this.isAdaptiveCards = isAdaptiveCards; | ||
| this.mentionCommitters = mentionCommitters; | ||
| this.mentionDevelopers = mentionDevelopers; |
There was a problem hiding this comment.
if you set commiters AND developers to true you will get committers list duplicated - is that correct?
committer is a subset of developers
There was a problem hiding this comment.
The original intent was to allow some flexibility in terms of who you wanted to mention. Some people might want to mention Developers or Committers. Knowing that committers is essentially a blame list for who Jenkins thinks broke the build, it might make more sense to just have committers be mentioned and not developers? As far as I'm aware anyone's name in the developer section will show up in the committer section.
|
Currently on vacation, will come back to this in a week or so |
fb7a821 to
7c72728
Compare
|
|
||
| Run previousBuild = run.getPreviousBuild(); | ||
| Result previousResult = previousBuild != null ? previousBuild.getResult() : Result.SUCCESS; | ||
| Result previousResult = previousBuild != null ? previousBuild.getResult() : Result.SUCCESS; |
There was a problem hiding this comment.
space at the end of the line - please clean up the files before pushing to code review
| .collect(Collectors.joining(", ")); | ||
| .sorted(Comparator.comparing(User::getFullName)) | ||
| .map(user -> mentionCommitters ? TeamsMentionUtils.mentionUserOrEmail(user) : user.getFullName()) | ||
| .filter(StringUtils::isNotBlank) // remove nulls or empty strings |
There was a problem hiding this comment.
this does not filter out string Unknown user while IMO should
| <f:checkbox/> | ||
| </f:entry> | ||
|
|
||
| <f:entry title="Mention Only When Build Has Failed" field="mentionOnFailure"> |
There was a problem hiding this comment.
or is unstable - at least this is how I understand the code above
| } | ||
|
|
||
| public static String mentionUserOrEmail(User user) { | ||
| if (user == null) { |
There was a problem hiding this comment.
when this condition is met on runtime?
There was a problem hiding this comment.
You're right, double checked that getCulprits() never returns null Users based on Jenkins core behavior. Removed the null check from TeamsMentionUtils.mentionUserOrEmail() since it's only used on the list produced by getCulprits. Test functionality for this null check has been removed as well. If you think re-implementing it differently is of use, please let me know.
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>display-url-api</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
do we need it?
it introduces security issue https://app.snyk.io/org/damianszczepanik/project/c78d3196-4d6a-4a74-a217-6f6bc5b2f6ac/pr-check/bbbd388e-00e1-4168-a972-a6579dd7772e
There was a problem hiding this comment.
I was wondering what that was. I couldn't (and still can't) see the security issue provided by snyk. But Good point. Mailer is not strictly required for this plugin. I can remove the hard dependency and switch to runtime detection so mentions use email when Mailer is present, and fall back to usernames otherwise. Would that work?
There was a problem hiding this comment.
Okay, surprised that work haha, I've set mailer to a test dependency now. Fallback behaviour will default if the mailer isn't there, but I needed to test integration. Looks good!
… so the users with extracted emails are tagged
…velopers on started cards
…ed mention failure text in jelly.config
…d in the shouldMention boolean
…mentioning comitters on failures
…ions.json and mocking committers in sampleIT not developers
…nd making sure developer names are listed properly
3d9f5a5 to
5cacf78
Compare
|
Noticed the build was failing because of a banned import (commons.lang.StringUtils), specified lang3 in TeamsMentionUtils to be consistent with what we use in factsbuilder.java |
Summary
How it Works / Idea
The goal of this feature is to allow Jenkins Office 365 Connector notifications to mention the users responsible for a build (committers or developers) in Microsoft Teams messages.
Workflow:
Collect User Information
hudson.model.User).Format Mentions
<at>user@example.com</at>tags usingTeamsMentionUtils.Conditional Mentions
mentionOnFailure).Send Notification
<at>tags and converts them into proper mention entities.Summary Flow:
The changes include:
New Configurable Options:
mentionOnFailure– Whether mentions are sent on build failures (default:true).CardBuilder Updates:
mentionCommittersandmentionOnFailureparameters.createCompletedCardto conditionally add mentions based on these parameters.FactsBuilder Updates:
addCommittersnow accepts a boolean to control mentions.Webhook and Jelly Config Updates:
mentionOnFailureproperties toWebhook.Utility and Tests:
TeamsMentionUtilsto wrap emails in<at>tags for Teams mentions.TeamsMentionUtilsto verify behavior for users with/without emails, and email formatting.Testing done
Unit Tests:
TeamsMentionUtilsTestto verify proper handling of user mentions and email formatting.Manual Verification:
mentionOnFailureenabled/disabled.Related Issues / PRs
#55
#416
Submitter checklist