[JENKINS-39203] Mark publishIssues steps with QualityGateStatus.WARNING with WarningAction #58
Conversation
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
|
|
||
| <!-- Override properties defined in parent POM --> | ||
| <jenkins.version>2.121.1</jenkins.version> | ||
| <jenkins.version>2.138.4</jenkins.version> |
There was a problem hiding this comment.
Required by the new version of workflow-api.
| return publisher.attachAction(); | ||
| ResultAction resultAction = publisher.attachAction(); | ||
| QualityGateStatus status = resultAction.getResult().getQualityGateStatus(); | ||
| if (status == QualityGateStatus.WARNING) { |
There was a problem hiding this comment.
Note that this would only improve the visualization if the status is WARNING. As far as I can tell, if the status is FAILED, then the stage containing the step won't be visualized as failing by Blue Ocean currently, which also seems undesirable. Is there any particular reason why the step doesn't throw an AbortException if it sets the build result to Failure to abort execution and mark the source of the failure?
There was a problem hiding this comment.
I don't throw an AbortException since there are other post build steps running afterwards (or static analysis results collected, see https://github.com/jenkinsci/warnings-ng-plugin/blob/master/src/main/java/io/jenkins/plugins/analysis/core/steps/IssuesRecorder.java#L535).
Users of my plugin typically collect several results. They want to see which individual result has a status of Success, Unstable or Failure. If the first is a Failure then the rest still can be Success. So I wonder why you do not have a similar concept for failing a step? Using only the Unstable status seems to be only half of the wanted solution. Visualization in blue ocean is also not really relevant here, since most of my users use the classic view, since they want to see the results of the analysis.
There was a problem hiding this comment.
There was a problem hiding this comment.
So I wonder why you do not have a similar concept for failing a step?
In a sense that is what I am trying to add via WarningAction to address JENKINS-39203/JENKINS-43995 - Some way to mark a step as having an error even if it didn't really have an error, because right now steps only fail if they return an exception via StepContext.onFailure. If that doesn't happen, then as far as visualizations are concerned the step succeeded.
In jenkinsci/workflow-api-plugin#91 I wrote:
In the future, if we wanted to add the ability to track some new kind of non-fatal result, we could update WarningAction to store a custom Result (or similar) and use that in visualizations to modify how the action should be displayed.
I could go ahead and add that functionality so that we could also handle Failure with WarningAction, which would address JENKINS-54373. My only hesitation is that it seems like failures as reported by this step are exactly the use case intended to be handled by the Unstable result, but I guess it's too late to change that and if the current behavior is what users want then the intended use cases for the Unstable/Failure results are kind of irrelevant.
Visualization in blue ocean is also not really relevant here, since most of my users use the classic view, since they want to see the results of the analysis.
Fair enough. For what it's worth this would also fix the visualization in the Pipeline Steps view (see jenkinsci/workflow-api-plugin#91 (comment)) and it would allow the visualization to be improved for Stage View (although I do not currently have plans to fix that myself).
There was a problem hiding this comment.
I could go ahead and add that functionality so that we could also handle Failure with WarningAction, which would address JENKINS-54373. My only hesitation is that it seems like failures as reported by this step are exactly the use case intended to be handled by the Unstable result, but I guess it's too late to change that and if the current behavior is what users want then the intended use cases for the Unstable/Failure results are kind of irrelevant.
I'm not sure if I understand correctly (I'm coming from the Freestyle/UI side and use pipelines not very often). If I have several (parallel) stages that e.g. compile for different platforms then I would like to see for each stage the individual result of (Success, Unstable, Failure). Even if I get a compile error in one stage I want to compile the rest and would like to see the successful results as well. This should be similar to maven multi module builds. Even if there is one failed module I might be still interested in the actual results of the other modules (option --fail-at-end).
BTW: I would only use Unstable for analysis results by myself. Just several of my users requested to use the Failure status as well for the quality gate.
There was a problem hiding this comment.
If I have several (parallel) stages that e.g. compile for different platforms then I would like to see for each stage the individual result of (Success, Unstable, Failure).
Yes, WarningAction as it exists now would make this work for Success and Unstable, but would need some additions to work for Failure. I am happy to make those additions if you think it would be useful.
Even if I get a compile error in one stage I want to compile the rest and would like to see the successful results as well. This should be similar to maven multi module builds. Even if there is one failed module I might be still interested in the actual results of the other modules (option --fail-at-end)
The default behavior of the parallel step already matches --fail-at-end: It waits until all branches complete before moving on, so even if one branch fails they would all execute to completion (and if users want the parallel step to fail immediately if a branch fails there is an option to do so). If you run analyses sequentially, then you do need something like WarningAction to keep the build from stopping at the first failure while still being able to see which step failed.
|
I'm not quite sure why the build is failing, but here's the error:
|
| return publisher.attachAction(); | ||
| ResultAction resultAction = publisher.attachAction(); | ||
| QualityGateStatus status = resultAction.getResult().getQualityGateStatus(); | ||
| if (status == QualityGateStatus.WARNING) { |
There was a problem hiding this comment.
I don't throw an AbortException since there are other post build steps running afterwards (or static analysis results collected, see https://github.com/jenkinsci/warnings-ng-plugin/blob/master/src/main/java/io/jenkins/plugins/analysis/core/steps/IssuesRecorder.java#L535).
Users of my plugin typically collect several results. They want to see which individual result has a status of Success, Unstable or Failure. If the first is a Failure then the rest still can be Success. So I wonder why you do not have a similar concept for failing a step? Using only the Unstable status seems to be only half of the wanted solution. Visualization in blue ocean is also not really relevant here, since most of my users use the classic view, since they want to see the results of the analysis.
| ResultAction resultAction = publisher.attachAction(); | ||
| QualityGateStatus status = resultAction.getResult().getQualityGateStatus(); | ||
| if (status == QualityGateStatus.WARNING) { | ||
| WarningAction action = new WarningAction("Some quality gates have been missed: overall result is " + status); |
There was a problem hiding this comment.
We should have a subclass of WarningAction here (or your action should be an interface?). Otherwise I can't provide a localized message. So WarningAction.getMessage() can be overwritten with a localized string.
There was a problem hiding this comment.
That's a good point. Note that as of right now, the message for WarningAction is not actually displayed/used anywhere. That said, I imagine it would be used in contexts such as visualizations, where a localized message would probably make sense rather than in something like builds logs where it would make less sense. Let me think about whether to make WarningAction itself take optionally take a Localizable or if plugins should subclass the action. In some cases WarningAction will be created from user-generated text, so those cases wouldn't be localizable, but I'd guess that plugins would like to be able to provide localizable messages when they use the actions.
There was a problem hiding this comment.
I'm not sure if using a Localizable property in a serializable action is a good thing (will be part of build.xml).
There was a problem hiding this comment.
The Localizable class and its members are Serializable, but yeah perhaps it's not the best idea to actually serialize it.
There was a problem hiding this comment.
For now, I made WarningAction non-final, so it could be extended here to support localization, although there are still no uses of the message so I'm not sure what contexts it would be displayed in and whether localization would make sense in those contexts.
| WarningAction action = new WarningAction("Some quality gates have been missed: overall result is " + status); | ||
| getContext().get(FlowNode.class).addOrReplaceAction(action); | ||
| } | ||
| return resultAction; |
There was a problem hiding this comment.
I think it makes more sense if this code is called in IssuePublisher, since most users (especially for declarative pipeline) use the IssueRecorder step.
So here: https://github.com/jenkinsci/warnings-ng-plugin/blob/master/src/main/java/io/jenkins/plugins/analysis/core/steps/IssuesPublisher.java#L145
Or here: https://github.com/jenkinsci/warnings-ng-plugin/blob/master/src/main/java/io/jenkins/plugins/analysis/core/util/QualityGateStatus.java#L58
I don't know if this FlowNode is available for a SimpleBuildStep?
There was a problem hiding this comment.
Yeah, that was what I had in mind with this comment from the PR description:
Another idea I had was to create some kind of
ResultHandlerclass that can be passed toIssuePublisherand then to a newQualityGateStatus.setResult(ResultHandler)method, but I wasn't sure if we wanted to make those kinds of changes ifpublishIssuesis the only caller that would actually useResultHandler.
I don't know if this FlowNode is available for a SimpleBuildStep
Unfortunately no, I don't think there is any way to make this work without changing the API of SimpleBuildStep (and I don't think that is likely to happen). When used in a Pipeline, SimpleBuildSteps are actually executed by CoreStep, so they have no way to get the current FlowNode because they don't have access to any Pipeline or Step-specific APIs. (We could add a compile-scope dependency on workflow-job and check run instanceof WorkflowRun, then use WorkflowRun.getExecution().getCurrentHeads() to try to find a FlowNode for CoreStep where CoreStep.delegate == this, but I'd consider that a serious hack.)
There was a problem hiding this comment.
Hmm, that would prevent all plugins that just implement SimpleBuildStep (there are a lot of them) not aware of the new concept 😢
Maybe it would make sense to provide an additional action SimpleBuildStep2 that provides the missing parameter FlowNode?
There was a problem hiding this comment.
Well, I think the idea behind SimpleBuildStep is that it can work in various kinds of jobs, so it can't have any behaviors that are specific to a certain job type (and it is defined in core, so I'm not sure we even could provide a SimpleBuildStep2 that takes a FlowNode as a parameter. Perhaps we could provide a new overload of SimpleBuildStep.perform that returns some composite type with a result and a message, and handle that in CoreStep, but I'm not sure we want to create a new core API just for this).
I guess the crux of the issue is that tracking per-step failures is very much a Pipeline-specific behavior, but it seems like you are using SimpleBuildStep intentionally to avoid needing to create a separate step just for Pipeline since the majority of the behavior is agnostic to the specifics of the build. I'm not sure if we can do anything easily to fix that without either an API change in Jenkins itself or creating a new step here that uses IssueRecorder internally (perhaps a Step with the same symbol name as IssueRecorder would take precedence when evaluated in Pipeline allowing transparent migration for users?).
There was a problem hiding this comment.
I see. Creating another step seems like a simpler solution. I already extracted the behavior of IssueRecorder and ScanForIssuesStep and PublishIssuesStep into separate classes in order to avoid duplication. Adding another step would not harm much (a lot of duplication will be required for data-binding of the fields though).
(perhaps a Step with the same symbol name as IssueRecorder would take precedence when evaluated in Pipeline allowing transparent migration for users?)
Shouldn't moving the symbol from IssueRecorder to a new step should do the trick?
There was a problem hiding this comment.
Shouldn't moving the symbol from IssueRecorder to a new step should do the trick?
Yeah that would be the easiest approach as long as the only uses of the symbol today are in Pipeline and so we wouldn't break anything for anyone else.
There was a problem hiding this comment.
Ok, I've just force-pushed with a new commit that introduces RecordIssuesStep, which is related to IssuesRecorder similarly to the way PublishIssuesStep relates to IssuesPublisher. Care to take a look to see if the overall approach seems ok to you? (I'd still need to finalize a few things like jelly files for the new step, but figured I'd wait to do that unless you agree that the approach seems ok).
EDIT: It would also be good to know if you like the way that I moved the actual result handling into QualityGateStatus by passing a possibly-null FlowNode around. We could use an opaque interface equivalent to Consumer<QualityGateStatus> instead if you'd rather not have FlowNode references directly in the classes or anticipate further extensions in the future.
There was a problem hiding this comment.
I think it would be better if we would rather have a new Interface that has two different implementations: one sets the status of the Run and the other additionally sets the status of the FlowNode. Then we can avoid null invocations.
There was a problem hiding this comment.
The overall approach of copying the recorder to a new step class looks ok. I think we can't avoid the duplication here (this is a similar and ugly thing that I already had with the maven plugin support and the scan and publish steps).
.mvn/extensions.xml
Outdated
| @@ -0,0 +1,7 @@ | |||
| <extensions xmlns="http://maven.apache.org/EXTENSIONS/1.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/EXTENSIONS/1.0.0 http://maven.apache.org/xsd/core-extensions-1.0.0.xsd"> | |||
| <extension> | |||
| <groupId>io.jenkins.tools.incrementals</groupId> | |||
There was a problem hiding this comment.
Nice! Then I can try to use incrementals for the analyis-model-api as well...
| FlowNode publishIssuesNode = new DepthFirstScanner().findFirstMatch(run.getExecution(), | ||
| node -> node.getDisplayFunctionName().equals("publishIssues")); | ||
| assertThat(publishIssuesNode).isNotNull(); | ||
| assertThat(publishIssuesNode.getPersistentAction(WarningAction.class)).isNotNull(); |
There was a problem hiding this comment.
If we would have a subclass for WarningAction then we could actually check for the concrete type and see that this action if from the warnings-ng plugin.
There was a problem hiding this comment.
Yeah, we could also check that the message is the one defined in this plugin.
|
BTW: The checkstyle error is caused by activating incrementals. I extracted that part into a separate branch, see CI log. Is this a bug in the incrementals plugin? |
Not that I'm aware of, but I haven't seen an error like this before so I'm not sure. I'll look into it. Edit: See #58 (comment). |
.mvn/maven.config
Outdated
| @@ -0,0 +1 @@ | |||
| -Pconsume-incrementals | |||
There was a problem hiding this comment.
Normally this would also have -Pmight-produce-incrementals, but after investigating #58 (comment) I removed that line as a workaround.
The issue in that comment does appear to be caused indirectly by using incrementals, though not by incrementals-maven-plugin itself. It looks like what is happening is that maven-checkstyle-plugin uses the project base directory to resolve relative paths (etc/checkstyle-configuration.xml). However, incrementals activates the might-produce-incrementals Maven profile, which uses flatten-maven-plugin to create an incrementals-suitable POM and sets this POM as the POM for the project, which appears to implicitly change the project's base directory to target/ (see mojohaus/flatten-maven-plugin#53 and related issues), which means that etc/checkstyle-configuration.xml is nowhere to be found. I think (but have not confirmed via testing) that the root issue is MNG-6405, for which a fix has been merged but not released.
I think we can remove this property and retain the ability to use incremental dependencies, but this plugin itself will not have incremental versions published, which seems like an ok short-term compromise until a new version of Maven is released.
There was a problem hiding this comment.
Thinking about this some more, we might need to revert the changes to pom.xml that switch the version number to ${revision}${changelist} since we aren't intending to produce incrementals, but now that the downstream changes have been released it's probably easier to just back out the incrementalification and handle it separately so we can make sure we fully understand the problem.
There was a problem hiding this comment.
That is ok for me.
My plan was to use (analysis-model) and produce incrementals anyway some time in the future so we can stash these changes until the maven problem has been released (and I find some spare time to enable that in the analysis-model projects as well).
pom.xml
Outdated
| <url>https://wiki.jenkins.io/x/1YTeCQ</url> | ||
| <description>Jenkins Warnings Next Generation plugin collects compiler warnings or issues reported by static | ||
| analysis tools and visualizes the results. It has built-in support for numerous static analysis tools | ||
| (including several compilers), see the list of supported report formats. | ||
| </description> | ||
|
|
||
| <properties> | ||
| <revision>5.1.0</revision> |
There was a problem hiding this comment.
RevAPI points out among many other things that QualityGateStatusHandler is a new public class and so we need to bump the minor version (although I guess we could make the class package-private if desired).
pom.xml
Outdated
| <code>java.*</code> | ||
| <regex>true</regex> | ||
| <classQualifiedName>jenkins.*</classQualifiedName> | ||
| <justification>Changes in Jenkins core are not part of this plugin's API</justification> |
There was a problem hiding this comment.
This and the hudson.* exclusions are rather broad and at least hudson.* probably picks up some classes from detached plugins that are no longer in core, but otherwise it looked like the 2.121.x to 2.138.x core update would've involved maybe 50 individual ignores. I guess some of the other exclusions should be cleaned up now that they are covered by these catch-alls.
It doesn't really make sense to me to consider changes in Jenkins core (or any other provided-scope dependency) as part of the API of a plugin from the point of view of semantic versioning, since any plugin that depends on this plugin will itself have to depend on the same version of Jenkins core or newer, and the plugin is not providing Jenkins' classes, Jenkins is, so it would be Jenkins that would need to be semantically versioned.
Ideally we'd be able to add ignores on a Maven artifact level rather than a class level but it didn't look like that was possible.
It almost seems like it would make more sense to set checkDependencies to false and only consider changes to this plugin itself to avoid needing all of these exclusions.
There was a problem hiding this comment.
Yes, I enabled that checker a couple of weeks ago and I am still not sure how to configure it in the right way for a Jenkins plugin. It works quite well with enabled dependencies for my analysis-model package, however here I'm still experimenting. I think disabling dependencies is a good alternative to get the focus on the changes in the plugin.
|
@dwnusbaum If you need some help on this please let me know. I can add the jelly files (and adapt some integration tests) so that this new step is correctly usable. |
|
@uhafner Thanks! I haven't had time to look at this the past few days, but I should have time to finish updating it tomorrow now that the downstream changes are released. Does that sound good to you, or would you rather take it from here yourself? |
|
I you have the time to finish it that would be wonderful! |
… WarningAction This commit introduces a new Step called RecordIssuesStep that is nearly identical to IssuesRecorder, but is able to retrieve the current FlowNode via Pipeline-specific APIs.
There was a problem hiding this comment.
@uhafner Ok, I've removed the incremental-related changes and added the Jelly files. Are there any particular test-related changes that you'd like me to make? All tests that were using recordIssues have switched over to using the new step transparently, so we do have some coverage other than the tests I added specifically about unstable status.
| <?jelly escape-by-default='true'?> | ||
| <j:jelly xmlns:j="jelly:core" xmlns:i="/issues" xmlns:f="/lib/form"> | ||
|
|
||
| <link rel="stylesheet" href="${resURL}/plugin/warnings-ng/css/custom-style.css"/> |
There was a problem hiding this comment.
I copied these over straight from IssuesRecorder and it looks like they work fine with no changes in the Pipeline step generator. We might be able to do something clever along the lines of:
<st:include class="io.jenkins.plugins.analysis.core.steps.IssuesRecorder" page="config.jelly"/>
... to avoid duplicating config.jelly, but I couldn't figure out a way to avoid duplicating the help files, and the complexity probably isn't worth it.
|
Not sure what the RevAPI failures are about, I might've forgotten to recheck them after rebasing on master. |
|
You remove the Revapi part in your pom.xml. I can reactivate it later on... |
|
@uhafner I think I figured out what was wrong, but if RevAPI still isn't happy I'll disable it for now 😄 |
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
============================================
- Coverage 83.02% 82.31% -0.72%
- Complexity 1512 1516 +4
============================================
Files 239 241 +2
Lines 5079 5213 +134
Branches 399 403 +4
============================================
+ Hits 4217 4291 +74
- Misses 712 771 +59
- Partials 150 151 +1
Continue to review full report at Codecov.
|
src/main/java/io/jenkins/plugins/analysis/core/util/QualityGateStatusHandler.java
Show resolved
Hide resolved
src/main/java/io/jenkins/plugins/analysis/core/util/QualityGateStatus.java
Outdated
Show resolved
Hide resolved
|
@uhafner I added some unit tests for |
|
Any estimate on when we can expect this to be released? |
|
It will be part of the next release. I still need to do some additional manual testing though. (And need to think on how to handle the Job-DSL migration since the freestyle publisher now requires to get a new symbol name.) |
|
@uhafner - @dwnusbaum is working on blog post for this feature, it would be great to be able to point people to an updated released version instead of this PR. |
|
The release is almost done. I just need to wait for https://ci.jenkins.io/job/Plugins/job/warnings-ng-plugin/job/master/471/ to finish successfully. Then I will release 5.2.0 this evening... |
See jenkinsci/workflow-api-plugin#91 and JENKINS-39203. (I am intentionally not updating the status of that ticket or assigning it to myself until I am ready to merge/release all relevant changes to avoid giving users premature hope of a fix and so that I have clear answers for what will and won't work and when any fixes will be released.)
This PR adds a
WarningActionto theStepAtomNodeforpublishIssuessteps that complete withQualityGateStatus.WARNINGso that the actual stage containing thepublishIssuesstep can be visualized as unstable via jenkinsci/pipeline-graph-analysis-plugin#25. The PR also incrementalifies the plugin (See JEP-305 for details) in order to consume an incremental dependency of workflow-api with the newWarningActionclass.As a first pass the new logic is just baked directly into
PublishIssuesStepbecause it's Pipeline-specific. Another idea I had was to create some kind ofResultHandlerclass that can be passed toIssuePublisherand then to a newQualityGateStatus.setResult(ResultHandler)method, but I wasn't sure if we wanted to make those kinds of changes ifpublishIssuesis the only caller that would actually useResultHandler. I added a basic integration test to show that the action is added, but if you think it has low value I am happy to delete it or adjust it as desired.