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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-39203] Mark publishIssues steps with QualityGateStatus.WARNING with WarningAction #58

Merged
merged 5 commits into from
Jun 21, 2019
Merged

[JENKINS-39203] Mark publishIssues steps with QualityGateStatus.WARNING with WarningAction #58

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c45a38
[JENKINS-39203] Mark failed publishIssues and recordIssues steps with…
dwnusbaum Apr 24, 2019
f2df273
Re-add RevAPI ignore for IssueParser return type changes
dwnusbaum May 16, 2019
8209b48
Add unit tests for QualityGateStatusHandlerTest and remove QualityGat…
dwnusbaum May 29, 2019
bc5b9b6
Merge branch 'master' into JENKINS-39203
dwnusbaum May 29, 2019
8eccb30
Resolve/suppress Codacy warnings
dwnusbaum May 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
62 changes: 15 additions & 47 deletions pom.xml
View file
Open in desktop
Expand Up @@ -4,8 +4,8 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.42</version>
<relativePath />
<version>3.43</version>
<relativePath/>
</parent>

<groupId>io.jenkins.plugins</groupId>
Expand All @@ -23,7 +23,7 @@
<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>
Copy link
Member Author

Choose a reason for hiding this comment

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

Required by the new version of workflow-api.

<java.level>8</java.level>
<jenkins-test-harness.version>2.49</jenkins-test-harness.version>

Expand Down Expand Up @@ -59,6 +59,7 @@
<matrix-project.version>1.13</matrix-project.version>
<junit-plugin.version>1.24</junit-plugin.version>
<git-plugin.version>3.9.1</git-plugin.version>
<workflow-api.version>2.34</workflow-api.version>
<workflow-step.version>2.19</workflow-step.version>
<workflow-job.version>2.32</workflow-job.version>
<structs.version>1.17</structs.version>
Expand Down Expand Up @@ -246,6 +247,11 @@
<artifactId>script-security</artifactId>
<version>${script-security.version}</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-api</artifactId>
<version>${workflow-api.version}</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
Expand Down Expand Up @@ -481,12 +487,6 @@
</dependency>

<!-- Declarative Pipeline -->
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-api</artifactId>
<version>2.33</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-support</artifactId>
Expand Down Expand Up @@ -738,6 +738,8 @@
</dependency>
</dependencies>
<configuration>
<!-- Including provided-scope dependencies like Jenkins core results in too many false positives -->
<checkDependencies>false</checkDependencies>
<analysisConfiguration>
<revapi.semver.ignore>
<enabled>true</enabled>
Expand All @@ -748,49 +750,15 @@
<newType>edu.hm.hafner.analysis.IssueParser</newType>
<justification>The parser should not be visible.</justification>
</item>
</revapi.ignore>
<revapi.ignore>
<item>
<regex>true</regex>
<code>java.missing.*</code>
<classQualifiedName>hudson.matrix.*</classQualifiedName>
<justification>Optional dependency</justification>
</item>
<item>
<code>java.class.nonPublicPartOfAPI</code>
<classQualifiedName>org.jvnet.hudson.reactor.Reactor.Node</classQualifiedName>
<justification>Not relevant</justification>
</item>
<item>
<code>java.class.nonPublicPartOfAPI</code>
<classQualifiedName>hudson.remoting.Command</classQualifiedName>
<justification>Not relevant</justification>
</item>
<item>
<code>java.class.nonPublicPartOfAPI</code>
<classQualifiedName>hudson.remoting.CommandTransport.CommandReceiver</classQualifiedName>
<justification>Not relevant</justification>
</item>
<item>
<code>java.class.nonPublicPartOfAPI</code>
<classQualifiedName>hudson.model.FingerprintMap.FingerprintParams</classQualifiedName>
<justification>Not relevant</justification>
</item>
<item>
<code>java.class.nonPublicPartOfAPI</code>
<classQualifiedName>org.kohsuke.stapler.AncestorImpl</classQualifiedName>
<justification>Not relevant</justification>
<justification>Dependencies are not being checked, so they are reported as missing</justification>
</item>
<item>
<code>java.class.nonPublicPartOfAPI</code>
<classQualifiedName>net.sf.json.AbstractJSON.WritingVisitor</classQualifiedName>
<justification>Not relevant</justification>
</item>
<item>
<code>java.class.nonPublicPartOfAPI</code>
<regex>true</regex>
<classQualifiedName>com.fasterxml.jackson.databind.*</classQualifiedName>
<justification>Not relevant</justification>
<code>java.annotation.removed</code>
<classQualifiedName>io.jenkins.plugins.analysis.core.steps.IssuesRecorder.Descriptor</classQualifiedName>
<justification>Symbol-based usage migrated transparently to RecordIssuesStep</justification>
</item>
</revapi.ignore>
</analysisConfiguration>
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/io/jenkins/plugins/analysis/core/steps/IssuesAggregator.java
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@

import io.jenkins.plugins.analysis.core.model.AnalysisResult;
import io.jenkins.plugins.analysis.core.model.ResultAction;
import io.jenkins.plugins.analysis.core.util.QualityGateStatusHandler;

/**
* Aggregates the {@link AnalysisResult}s of all {@link ResultAction}s of several {@link MatrixRun}s into {@link
Expand Down Expand Up @@ -91,7 +92,8 @@ private void updateMap(final List<ResultAction> actions) {
public boolean endBuild() {
for (Entry<String, List<AnnotatedReport>> reportsPerId : results.entrySet()) {
AnnotatedReport aggregatedReport = new AnnotatedReport(reportsPerId.getKey(), reportsPerId.getValue());
recorder.publishResult(build, listener, Messages.Tool_Default_Name(), aggregatedReport, StringUtils.EMPTY);
recorder.publishResult(build, listener, Messages.Tool_Default_Name(), aggregatedReport, StringUtils.EMPTY,
new QualityGateStatusHandler.SetBuildResultStatusHandler(build));
}
return true;
}
Expand Down
8 changes: 6 additions & 2 deletions src/main/java/io/jenkins/plugins/analysis/core/steps/IssuesPublisher.java
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@
import io.jenkins.plugins.analysis.core.util.LogHandler;
import io.jenkins.plugins.analysis.core.util.QualityGateEvaluator;
import io.jenkins.plugins.analysis.core.util.QualityGateStatus;
import io.jenkins.plugins.analysis.core.util.QualityGateStatusHandler;

import static io.jenkins.plugins.analysis.core.model.AnalysisHistory.JobResultEvaluationMode.*;
import static io.jenkins.plugins.analysis.core.model.AnalysisHistory.QualityGateEvaluationMode.*;
Expand All @@ -48,12 +49,14 @@ class IssuesPublisher {
private final QualityGateEvaluationMode qualityGateEvaluationMode;
private final JobResultEvaluationMode jobResultEvaluationMode;
private final LogHandler logger;
private final QualityGateStatusHandler qualityGateStatusHandler;

@SuppressWarnings("ParameterNumber")
IssuesPublisher(final Run<?, ?> run, final AnnotatedReport report,
final HealthDescriptor healthDescriptor, final QualityGateEvaluator qualityGate,
final String name, final String referenceJobName, final boolean ignoreQualityGate,
final boolean ignoreFailedBuilds, final Charset sourceCodeEncoding, final LogHandler logger) {
final boolean ignoreFailedBuilds, final Charset sourceCodeEncoding, final LogHandler logger,
final QualityGateStatusHandler qualityGateStatusHandler) {
this.report = report;
this.run = run;
this.healthDescriptor = healthDescriptor;
Expand All @@ -64,6 +67,7 @@ class IssuesPublisher {
qualityGateEvaluationMode = ignoreQualityGate ? IGNORE_QUALITY_GATE : SUCCESSFUL_QUALITY_GATE;
jobResultEvaluationMode = ignoreFailedBuilds ? NO_JOB_FAILURE : IGNORE_JOB_RESULT;
this.logger = logger;
this.qualityGateStatusHandler = qualityGateStatusHandler;
}

private String getId() {
Expand Down Expand Up @@ -142,7 +146,7 @@ private QualityGateStatus evaluateQualityGate(final Report filtered, final Delta
else {
filtered.logInfo("-> Some quality gates have been missed: overall result is %s", qualityGateStatus);
}
qualityGateStatus.setResult(run);
qualityGateStatusHandler.handleStatus(qualityGateStatus);
}
else {
filtered.logInfo("No quality gates have been set - skipping");
Expand Down
30 changes: 20 additions & 10 deletions src/main/java/io/jenkins/plugins/analysis/core/steps/IssuesRecorder.java
View file
Open in desktop
Expand Up @@ -16,7 +16,6 @@
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;
import org.kohsuke.stapler.QueryParameter;
import org.jenkinsci.Symbol;
import hudson.Extension;
import hudson.FilePath;
import hudson.Launcher;
Expand Down Expand Up @@ -52,6 +51,7 @@
import io.jenkins.plugins.analysis.core.util.QualityGate.QualityGateResult;
import io.jenkins.plugins.analysis.core.util.QualityGate.QualityGateType;
import io.jenkins.plugins.analysis.core.util.QualityGateEvaluator;
import io.jenkins.plugins.analysis.core.util.QualityGateStatusHandler;

/**
* Freestyle or Maven job {@link Recorder} that scans report files or the console log for issues. Stores the created
Expand All @@ -73,7 +73,7 @@
*/
@SuppressWarnings({"PMD.ExcessivePublicCount", "PMD.ExcessiveClassLength", "PMD.ExcessiveImports", "PMD.TooManyFields", "PMD.DataClass", "ClassDataAbstractionCoupling", "ClassFanOutComplexity"})
public class IssuesRecorder extends Recorder implements SimpleBuildStep {
private static final String NO_REFERENCE_JOB = "-";
static final String NO_REFERENCE_JOB = "-";

private List<Tool> analysisTools = new ArrayList<>();

Expand Down Expand Up @@ -284,7 +284,7 @@ public void setTool(final Tool tool) {
analysisTools = Collections.singletonList(tool);
}

private void ensureThatToolIsValid(final Tool tool) {
static void ensureThatToolIsValid(final Tool tool) {
if (tool == null) {
throw new IllegalArgumentException("No valid tool defined! You probably used a symbol in the tools "
+ "definition that is also a symbol in another plugin. "
Expand Down Expand Up @@ -504,9 +504,19 @@ public Descriptor getDescriptor() {
public void perform(@NonNull final Run<?, ?> run, @NonNull final FilePath workspace,
@NonNull final Launcher launcher, @NonNull final TaskListener listener)
throws InterruptedException, IOException {
perform(run, workspace, listener, new QualityGateStatusHandler.SetBuildResultStatusHandler(run));
}

/**
* Executes the build step. Used from {@link RecordIssuesStep} to provide a {@link QualityGateStatusHandler}
* that has Pipeline-specific behavior.
*/
void perform(@NonNull final Run<?, ?> run, @NonNull final FilePath workspace,
@NonNull final TaskListener listener, @NonNull final QualityGateStatusHandler statusHandler)
throws InterruptedException, IOException {
Result overallResult = run.getResult();
if (isEnabledForFailure || overallResult == null || overallResult.isBetterOrEqualTo(Result.UNSTABLE)) {
record(run, workspace, listener);
record(run, workspace, listener, statusHandler);
}
else {
LogHandler logHandler = new LogHandler(listener, createLoggerPrefix());
Expand All @@ -518,7 +528,8 @@ private String createLoggerPrefix() {
return analysisTools.stream().map(Tool::getActualName).collect(Collectors.joining());
}

private void record(final Run<?, ?> run, final FilePath workspace, final TaskListener listener)
private void record(final Run<?, ?> run, final FilePath workspace, final TaskListener listener,
final QualityGateStatusHandler statusHandler)
throws IOException, InterruptedException {
for (Tool tool : getTools()) {
ensureThatToolIsValid(tool);
Expand All @@ -529,7 +540,7 @@ private void record(final Run<?, ?> run, final FilePath workspace, final TaskLis
totalIssues.add(scanWithTool(run, workspace, listener, tool), tool.getActualId());
}
String toolName = StringUtils.defaultIfEmpty(getName(), Messages.Tool_Default_Name());
publishResult(run, listener, toolName, totalIssues, toolName);
publishResult(run, listener, toolName, totalIssues, toolName, statusHandler);
}
else {
for (Tool tool : analysisTools) {
Expand All @@ -543,7 +554,7 @@ private void record(final Run<?, ?> run, final FilePath workspace, final TaskLis
report.logInfo("Ignoring name='%s' and id='%s' when publishing non-aggregating reports",
name, id);
}
publishResult(run, listener, tool.getActualName(), report, getReportName(tool));
publishResult(run, listener, tool.getActualName(), report, getReportName(tool), statusHandler);
}
}
}
Expand Down Expand Up @@ -605,7 +616,7 @@ private Charset getCharset(final String encoding) {
*/
@SuppressWarnings("deprecation")
void publishResult(final Run<?, ?> run, final TaskListener listener, final String loggerName,
final AnnotatedReport report, final String reportName) {
final AnnotatedReport report, final String reportName, QualityGateStatusHandler statusHandler) {
QualityGateEvaluator qualityGate = new QualityGateEvaluator();
if (qualityGates.isEmpty()) {
qualityGates.addAll(QualityGate.map(thresholds));
Expand All @@ -614,7 +625,7 @@ void publishResult(final Run<?, ?> run, final TaskListener listener, final Strin
IssuesPublisher publisher = new IssuesPublisher(run, report,
new HealthDescriptor(healthy, unhealthy, minimumSeverity), qualityGate,
reportName, referenceJobName, ignoreQualityGate, ignoreFailedBuilds, getSourceCodeCharset(),
new LogHandler(listener, loggerName, report.getReport()));
new LogHandler(listener, loggerName, report.getReport()), statusHandler);
publisher.attachAction();
}

Expand Down Expand Up @@ -1030,7 +1041,6 @@ public int getFailedNewLow() {
* Descriptor for this step: defines the context and the UI elements.
*/
@Extension
@Symbol("recordIssues")
@SuppressWarnings("unused") // most methods are used by the corresponding jelly view
public static class Descriptor extends BuildStepDescriptor<Publisher> {
/** Retain backward compatibility. */
Expand Down
11 changes: 8 additions & 3 deletions src/main/java/io/jenkins/plugins/analysis/core/steps/PublishIssuesStep.java
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;
import org.kohsuke.stapler.QueryParameter;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.jenkinsci.plugins.workflow.steps.Step;
import org.jenkinsci.plugins.workflow.steps.StepContext;
import org.jenkinsci.plugins.workflow.steps.StepDescriptor;
Expand All @@ -38,6 +39,7 @@
import io.jenkins.plugins.analysis.core.util.QualityGate.QualityGateResult;
import io.jenkins.plugins.analysis.core.util.QualityGate.QualityGateType;
import io.jenkins.plugins.analysis.core.util.QualityGateEvaluator;
import io.jenkins.plugins.analysis.core.util.QualityGateStatusHandler;

/**
* Publish issues created by a static analysis build. The recorded issues are stored as a {@link ResultAction} in the
Expand Down Expand Up @@ -569,10 +571,13 @@ protected ResultAction run() throws IOException, InterruptedException, IllegalSt
}
report.addAll(reports);

QualityGateStatusHandler statusHandler = new QualityGateStatusHandler.PipelineStatusHandler(getRun(),
getContext().get(FlowNode.class));
IssuesPublisher publisher = new IssuesPublisher(getRun(), report, healthDescriptor, qualityGate,
name, referenceJobName, ignoreQualityGate, ignoreFailedBuilds,
getCharset(sourceCodeEncoding), getLogger(report));
return publisher.attachAction();
getCharset(sourceCodeEncoding), getLogger(report), statusHandler);
ResultAction resultAction = publisher.attachAction();
return resultAction;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ResultHandler class that can be passed to IssuePublisher and then to a new QualityGateStatus.setResult(ResultHandler) method, but I wasn't sure if we wanted to make those kinds of changes if publishIssues is the only caller that would actually use ResultHandler.

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.)

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@dwnusbaum dwnusbaum Apr 25, 2019 •
edited

Choose a reason for hiding this comment

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

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?).

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@dwnusbaum dwnusbaum May 6, 2019 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

}

private LogHandler getLogger(final AnnotatedReport report) throws InterruptedException {
Expand All @@ -590,7 +595,7 @@ public static class Descriptor extends StepDescriptor {

@Override
public Set<Class<?>> getRequiredContext() {
return Sets.immutable.of(Run.class, TaskListener.class).castToSet();
return Sets.immutable.of(FlowNode.class, Run.class, TaskListener.class).castToSet();
}

@Override
Expand Down