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

Add withChecks step #49

Merged
merged 11 commits into from
Dec 20, 2020
10 changes: 9 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,20 @@ If enabled, the statuses will be published in different stages of a Jenkins buil

### Pipeline Usage

Instead of depending on consumers plugins, the users can publish their checks directly in the pipeline script:
- publishChecks: you can publish checks directly in the pipeline script instead of depending on consumer plugins:

```
publishChecks name: 'example', title: 'Pipeline Check', summary: 'check through pipeline', text: 'you can publish checks in pipeline script', detailsURL: 'https://github.com/jenkinsci/checks-api-plugin#pipeline-usage'
```

- withChecks: you can inject the check's name into the closure for other steps to use:
XiongKezhi marked this conversation as resolved.
Show resolved Hide resolved

```
withChecks(name: 'injected name') {
// some other steps that will extract the name
}
```

timja marked this conversation as resolved.
Show resolved Hide resolved
## Guides

- [Consumers Guide](docs/consumers-guide.md)
Expand Down
20 changes: 20 additions & 0 deletions docs/consumers-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,23 @@ Consumers can set these parameters through the checks models:

The publishers are created through the static factory method (`fromRun` or `fromJob`) of `ChecksPublisherFactory`.
The factory will iterate all available implementations of the `ChecksPublisher` in order to find the suitable publisher for the Jenkins `Run` or `Job`.

## Pipeline Step: withChecks

The `withChecks` step injects a `ChecksInfo` object into its closure by users:

```groovy
withChecks('MyCheck') {
junit '*.xml'
}
```

The injected object can be resolved by other plugin developers in their [Step](https://javadoc.jenkins.io/plugin/workflow-step-api/org/jenkinsci/plugins/workflow/steps/Step.html) implementation:

```
getContext().get(ChecksInfo.class)
```

Currently, the `ChecksInfo` object only includes a `name` specified by users,
it is recommended that you look for this name and set it over your default checks name

8 changes: 6 additions & 2 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@

<properties>
<java.level>8</java.level>
<revision>1.1.2</revision>
<changelist>-SNAPSHOT</changelist>
<revision>1.2.0</revision>
Copy link
Member

Choose a reason for hiding this comment

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

It still should be a SNAPSHOT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sem-ver plugin will warn about compatibility if add SNAPSHOT


<!-- Jenkins Plug-in Dependencies Versions -->
<plugin-util-api.version>1.6.0</plugin-util-api.version>
Expand Down Expand Up @@ -56,6 +55,11 @@
<type>test-jar</type>
</dependency>

<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>display-url-api</artifactId>
</dependency>

<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
Expand Down
28 changes: 28 additions & 0 deletions src/main/java/io/jenkins/plugins/checks/steps/ChecksInfo.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package io.jenkins.plugins.checks.steps;

import java.io.Serializable;
import java.util.Objects;

/**
* A collection of checks properties that will be injected into {@link WithChecksStep} closure.
*/
public class ChecksInfo implements Serializable {
private static final long serialVersionUID = 1L;

private final String name;

/**
* Creates a {@link ChecksInfo} with checks name.
*
* @param name
* the name of the check
*/
public ChecksInfo(final String name) {
Objects.requireNonNull(name);
this.name = name;
}

public String getName() {
return name;
}
}
188 changes: 188 additions & 0 deletions src/main/java/io/jenkins/plugins/checks/steps/WithChecksStep.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
package io.jenkins.plugins.checks.steps;

import edu.hm.hafner.util.VisibleForTesting;
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import hudson.model.Run;
import hudson.model.TaskListener;
import io.jenkins.plugins.checks.api.*;
import io.jenkins.plugins.util.PluginLogger;
import org.jenkinsci.plugins.displayurlapi.DisplayURLProvider;
import org.jenkinsci.plugins.workflow.steps.*;
import org.kohsuke.stapler.DataBoundConstructor;

import java.io.IOException;
import java.io.Serializable;
import java.util.*;
import java.util.logging.Level;
import java.util.logging.Logger;

import static hudson.Util.fixNull;

/**
* Pipeline step that injects a {@link ChecksInfo} into the closure.
*/
public class WithChecksStep extends Step implements Serializable {
private static final long serialVersionUID = 1L;

private final String name;

/**
* Creates the step with a name to inject.
*
* @param name name to inject
*/
@DataBoundConstructor
public WithChecksStep(final String name) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

yes it can take more than one, normally you would use a databoundsetter for any optional parameters

Then the syntax would be:

withChecks('Tests') {
...
}
withChecks(name: 'Tests', myotherparam: 'hello') {
...
}

if there's only one mandatory field in constructor then you can omit the parameter name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should we make all params optional in case that in the future, some users may only want to inject a certain field like conclusion instead of name?

Copy link
Member

Choose a reason for hiding this comment

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

if we do that then the user will have to specify the name in a more verbose way,

withChecks(name: 'hello') rather than withChecks('hello')

Copy link
Contributor Author

@XiongKezhi XiongKezhi Dec 16, 2020

Choose a reason for hiding this comment

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

Specify the name wouldn't be annoying for users IMO but make it mandatory will be an issue for future changes?

Also, it seems the snippet generator will always specify name

@mrginglymus What do you think? Do you think there will be situations where you do not want to specify the name but the status (e.g. publish several checks in the same status (say in progress) but with different names in the closure)?

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 should be fine to be mandatory

super();

this.name = name;
}

public String getName() {
return name;
}

@Override
public StepExecution start(final StepContext stepContext) {
return new WithChecksStepExecution(stepContext, this);
}

/**
* This step's descriptor which defines the function name ("withChecks") and required context.
*/
@Extension
public static class WithChecksStepDescriptor extends StepDescriptor {
@Override
public String getFunctionName() {
return "withChecks";
}

@Override
public Set<? extends Class<?>> getRequiredContext() {
return Collections.unmodifiableSet(new HashSet<>(Arrays.asList(Run.class, TaskListener.class)));
}

@Override
public boolean takesImplicitBlockArgument() {
return true;
}

@NonNull
@Override
public String getDisplayName() {
return "Inject checks properties into its closure";
}
}

/**
* The step's execution to actually inject the {@link ChecksInfo} into the closure.
*/
static class WithChecksStepExecution extends AbstractStepExecutionImpl {
private static final long serialVersionUID = 1L;
private static final Logger SYSTEM_LOGGER = Logger.getLogger(WithChecksStepExecution.class.getName());

private final WithChecksStep step;

WithChecksStepExecution(final StepContext context, final WithChecksStep step) {
super(context);
this.step = step;
}

@Override
public boolean start() {
ChecksInfo info = extractChecksInfo();
getContext().newBodyInvoker()
.withContext(info)
.withCallback(new WithChecksCallBack(info))
.start();
return false;
}

@VisibleForTesting
ChecksInfo extractChecksInfo() {
return new ChecksInfo(step.name);
}

@Override
public void stop(final Throwable cause) {
publish(getContext(), new ChecksDetails.ChecksDetailsBuilder()
.withName(step.getName())
.withStatus(ChecksStatus.COMPLETED)
.withConclusion(ChecksConclusion.CANCELED));
}

private void publish(final StepContext context, final ChecksDetails.ChecksDetailsBuilder builder) {
TaskListener listener = TaskListener.NULL;
try {
listener = fixNull(context.get(TaskListener.class), TaskListener.NULL);
}
catch (IOException | InterruptedException e) {
SYSTEM_LOGGER.log(Level.WARNING, "Failed getting TaskListener from the context: " + e);
XiongKezhi marked this conversation as resolved.
Show resolved Hide resolved
}

PluginLogger pluginLogger = new PluginLogger(listener.getLogger(), "Checks API");

Run<?, ?> run;
try {
run = context.get(Run.class);
}
catch (IOException | InterruptedException e) {
String msg = "Failed getting Run from the context on the start of withChecks step: " + e;
pluginLogger.log(msg);
SYSTEM_LOGGER.log(Level.WARNING, msg);
context.onFailure(new IllegalStateException(msg));
return;
}

if (run == null) {
String msg = "No Run found in the context.";
pluginLogger.log(msg);
SYSTEM_LOGGER.log(Level.WARNING, msg);
context.onFailure(new IllegalStateException(msg));
return;
}

ChecksPublisherFactory.fromRun(run, listener)
.publish(builder.withDetailsURL(DisplayURLProvider.get().getRunURL(run))
.build());
}

class WithChecksCallBack extends BodyExecutionCallback {
private static final long serialVersionUID = 1L;

private final ChecksInfo info;

WithChecksCallBack(final ChecksInfo info) {
super();

this.info = info;
}

@Override
public void onStart(final StepContext context) {
publish(context, new ChecksDetails.ChecksDetailsBuilder()
.withName(info.getName())
.withStatus(ChecksStatus.IN_PROGRESS)
.withConclusion(ChecksConclusion.NONE));
}

@Override
public void onSuccess(final StepContext context, final Object result) {
context.onSuccess(result);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the support of the callback, this step may be much more useful (but I'm not sure if that goes beyond this step's scope) than just injecting the properties: it can be used as a progress reporter for the steps in Its closure.

Then, here is an issue:

  1. We cannot publish checks on success since we definitely don't want a simple success report check to overwrite what's inside the block (in the context of this API, we don't know about checks updating even if we support that in implementation).

  2. If we don't publish success, however, a simple closure in which no step publishes checks using the injected name will leave the check uncompleted forever. (as said before, if the progress reporter feature goes beyond the scope, then this should the users' problem)

Copy link
Member

Choose a reason for hiding this comment

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

If we don't publish success, however, a simple closure in which no step publishes checks using the injected name will leave the check uncompleted forever. (as said before, if the progress reporter feature goes beyond the scope, then this should the users' problem)

not our problem I think

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's reasonable to expect users to be responsible for sending a success message (or indeed a failure message) within their invocation of withChecks. I did wonder about adding to the publisher API the ability to get the current conclusion (https://github.com/jenkinsci/github-checks-plugin/pull/87/files#diff-9c475e3093c11ae9324debf6c824b9c15277a027428e5c7ed60ad6ac2ac25284R131-R134) so that we could query that in onSuccess and send a generic 'success' check if nothing else was set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems not feasible, since in the context of the API, we don't expect the implementation to possess the ChecksDetails alone the build, and if we take that as a users' responsible, there is no need to add the getConclusion method only for this feature IMO.

}

@Override
public void onFailure(final StepContext context, final Throwable t) {
publish(context, new ChecksDetails.ChecksDetailsBuilder()
.withName(info.getName())
.withStatus(ChecksStatus.COMPLETED)
.withConclusion(ChecksConclusion.FAILURE)
.withOutput(new ChecksOutput.ChecksOutputBuilder()
.withSummary("occurred while executing withChecks step.")
XiongKezhi marked this conversation as resolved.
Show resolved Hide resolved
.withText(t.toString()).build()));
context.onFailure(t);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* Provides default Findbugs annotations.
*/
@DefaultAnnotation(NonNull.class)
package io.jenkins.plugins.checks.steps;

import edu.umd.cs.findbugs.annotations.DefaultAnnotation;
import edu.umd.cs.findbugs.annotations.NonNull;
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:f="/lib/form">

<f:entry title="${%title.name}" field="name">
<f:textbox />
</f:entry>

</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
title.name=Name
Loading