[JENKINS-45471] - Generalize handling of scheduling errors and prevent NPEs in perform3() #115
Conversation
…t NPEs in perform3()
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| listener.error("Skipping " + jobToTrigger.getFullName() + "..."); | ||
| if (LOGGER.isLoggable(Level.CONFIG)) { | ||
| String message = String.format("Cannot schedule project %s. Job type is not parameterized, " | ||
| + "or there is no Item/Build permission for the current authentication %s. " |
There was a problem hiding this comment.
As I have mentioned previously, Item/Build is nonexistent. The Java constant is Item.BUILD. The permission as displayed in the UI is Job/Build.
| + "or there is no Item/Build permission for the current authentication %s. " | ||
| + "Skipping...", jobToTrigger, Jenkins.getAuthentication().getName()); | ||
| LOGGER.log(Level.CONFIG, String.format("%s: %s", run, message), | ||
| new UnsupportedOperationException("Cannot schedule job " + jobToTrigger.getFullName())); |
There was a problem hiding this comment.
Is a stack trace really necessary here? I do not see why.
| private void reportSchedulingError(@Nonnull Run<?, ?> run, @Nonnull Job<?, ?> jobToTrigger, @Nonnull BuildListener listener) { | ||
| // Do not print details to Build Listener, they have been reported previously in #canTriggerProject() | ||
| listener.error("Skipping " + jobToTrigger.getFullName() + "..."); | ||
| if (LOGGER.isLoggable(Level.CONFIG)) { |
There was a problem hiding this comment.
BuildTrigger and ReverseBuildTrigger print similar messages at WARNING. Is there a reason you are suppressing this message from logs by default? It is unlikely an admin will ever see it—by the time they figure out they need to be turning on this logger, they will already know about the problem and the fix. The point is to guide admins to help their users address configuration problems.
There was a problem hiding this comment.
@jglick ADMIN already gets a WARNING here: https://github.com/jenkinsci/parameterized-trigger-plugin/blob/master/src/main/java/hudson/plugins/parameterizedtrigger/BuildTriggerConfig.java#L571-L582
This logging with Exception exists for extra diagnostics purpose only
There was a problem hiding this comment.
OK…confusing but it sounds like this is just for debugging?
There was a problem hiding this comment.
Yep. Will add more comments during the logging update in the plugin
The change polishes #114 and adds check of another NPE risk in
BuildTriggerConfig#perform3(). Will add tests later... really...https://issues.jenkins-ci.org/browse/JENKINS-45471
@reviewbybees @fbelzunc