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

[WIP] workflow support? #26

Closed
wants to merge 16 commits into from
67 changes: 34 additions & 33 deletions pom.xml
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">

<modelVersion>4.0.0</modelVersion>

Expand Down Expand Up @@ -57,9 +58,9 @@
<dependency>
<groupId>com.coravy.hudson.plugins.github</groupId>
<artifactId>github</artifactId>
<version>1.14.0</version>
<version>1.14.2</version>
</dependency>

<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>github-api</artifactId>
Expand All @@ -81,7 +82,7 @@
<version>2.38.2</version>
<optional>true</optional>
</dependency>

<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>token-macro</artifactId>
Expand All @@ -106,35 +107,35 @@
<version>4.11</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>1.10.5</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-core</artifactId>
<version>${powermock.version}</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-module-junit4</artifactId>
<version>${powermock.version}</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-api-mockito</artifactId>
<version>${powermock.version}</version>
<scope>test</scope>
</dependency>

</dependencies>

<repositories>
Expand Down Expand Up @@ -190,32 +191,32 @@
</executions>
</plugin>
<!--<plugin>-->
<!--<groupId>org.apache.maven.plugins</groupId>-->
<!--<artifactId>maven-checkstyle-plugin</artifactId>-->
<!--<version>2.14</version>-->
<!--<executions>-->
<!--<execution>-->
<!--<id>validate</id>-->
<!--&lt;!&ndash;<phase>validate</phase>&ndash;&gt;-->
<!--<configuration>-->
<!--<configLocation>checkstyle.xml</configLocation>-->
<!--<suppressionsLocation>checkstyle-suppressions.xml</suppressionsLocation>-->
<!--&lt;!&ndash;<propertyExpansion>config_loc=${project.build.directory}/</propertyExpansion>&ndash;&gt;-->
<!--<includeTestSourceDirectory>true</includeTestSourceDirectory>-->
<!--<encoding>UTF-8</encoding>-->
<!--<consoleOutput>true</consoleOutput>-->
<!--<failsOnError>false</failsOnError>-->
<!--</configuration>-->
<!--<goals>-->
<!--<goal>check</goal>-->
<!--</goals>-->
<!--</execution>-->
<!--</executions>-->
<!--<groupId>org.apache.maven.plugins</groupId>-->
<!--<artifactId>maven-checkstyle-plugin</artifactId>-->
<!--<version>2.14</version>-->
<!--<executions>-->
<!--<execution>-->
<!--<id>validate</id>-->
<!--&lt;!&ndash;<phase>validate</phase>&ndash;&gt;-->
<!--<configuration>-->
<!--<configLocation>checkstyle.xml</configLocation>-->
<!--<suppressionsLocation>checkstyle-suppressions.xml</suppressionsLocation>-->
<!--&lt;!&ndash;<propertyExpansion>config_loc=${project.build.directory}/</propertyExpansion>&ndash;&gt;-->
<!--<includeTestSourceDirectory>true</includeTestSourceDirectory>-->
<!--<encoding>UTF-8</encoding>-->
<!--<consoleOutput>true</consoleOutput>-->
<!--<failsOnError>false</failsOnError>-->
<!--</configuration>-->
<!--<goals>-->
<!--<goal>check</goal>-->
<!--</goals>-->
<!--</execution>-->
<!--</executions>-->
<!--</plugin>-->
<!--<plugin>-->
<!--<groupId>org.apache.maven.plugins</groupId>-->
<!--<artifactId>maven-pmd-plugin</artifactId>-->
<!--<version>3.4</version>-->
<!--<groupId>org.apache.maven.plugins</groupId>-->
<!--<artifactId>maven-pmd-plugin</artifactId>-->
<!--<version>3.4</version>-->
<!--</plugin>-->
</plugins>
</build>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package org.jenkinsci.plugins.github.pullrequest;

import hudson.Extension;
import hudson.model.AbstractBuild;
import hudson.model.Run;
import hudson.model.TaskListener;
import hudson.model.listeners.RunListener;
import hudson.plugins.git.util.BuildData;
Expand All @@ -11,46 +11,48 @@
import javax.annotation.Nonnull;
import java.io.IOException;

import static org.jenkinsci.plugins.github.pullrequest.utils.JobHelper.ghPRTriggerFromRun;

/**
* Sets Pending build status before build run and manipulates Git's BuildData attached to job Action.
*
* @author Kanstantsin Shautsou
*/
@Extension
public class GitHubPRBuildListener extends RunListener<AbstractBuild<?, ?>> {
public class GitHubPRBuildListener extends RunListener<Run<?, ?>> {
private static final Logger LOGGER = LoggerFactory.getLogger(GitHubPRBuildListener.class);

@Override
public void onCompleted(AbstractBuild<?, ?> build, @Nonnull TaskListener listener) {
GitHubPRTrigger trigger = build.getProject().getTrigger(GitHubPRTrigger.class);
public void onCompleted(Run<?, ?> run, @Nonnull TaskListener listener) {
GitHubPRTrigger trigger = ghPRTriggerFromRun(run);
if (trigger == null) {
return;
}

GitHubPRCause cause = build.getCause(GitHubPRCause.class);
GitHubPRCause cause = run.getCause(GitHubPRCause.class);

if (cause != null) {
//remove all BuildData, because it doesn't work right with pull requests now
//TODO rework after git-client patching about BuildData usage
build.getActions().removeAll(build.getActions(BuildData.class));
run.getActions().removeAll(run.getActions(BuildData.class));
}
}

@Override
public void onStarted(AbstractBuild<?, ?> build, TaskListener listener) {
GitHubPRTrigger trigger = build.getProject().getTrigger(GitHubPRTrigger.class);
public void onStarted(Run<?, ?> run, TaskListener listener) {
GitHubPRTrigger trigger = ghPRTriggerFromRun(run);
if (trigger == null) {
return;
}

GitHubPRCause c = build.getCause(GitHubPRCause.class);
GitHubPRCause c = run.getCause(GitHubPRCause.class);
if (c == null) {
return;
}

// short build description shown in history
try {
build.setDescription("<a title=\"" + c.getTitle() + "\" href=\"" + c.getHtmlUrl() + "\">PR #"
run.setDescription("<a title=\"" + c.getTitle() + "\" href=\"" + c.getHtmlUrl() + "\">PR #"
+ c.getNumber() + "</a>: " + c.getAbbreviatedTitle());
} catch (IOException e) {
LOGGER.error("Can't set build description", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,22 @@
import hudson.model.AbstractBuild;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
import hudson.model.Run;
import hudson.model.TaskListener;
import jenkins.model.Jenkins;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.DataBoundConstructor;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.annotation.CheckForNull;
import java.io.IOException;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Represents a comment for GitHub that can contain token macros.
Expand All @@ -33,48 +38,61 @@ public GitHubPRMessage(String content) {
this.content = content;
}

public String expandAll(AbstractBuild<?, ?> build, TaskListener listener) {
String content = getContent();
/**
* Expand all what we can from given run
*/
@Restricted(NoExternalUse.class)
@CheckForNull
public String expandAll(Run<?, ?> run, TaskListener listener) throws IOException, InterruptedException {
return expandAll(getContent(), run, listener);
}

/**
* Expand all what we can from given run
*/
@Restricted(NoExternalUse.class)
@CheckForNull
public static String expandAll(String content, Run<?, ?> run, TaskListener listener)
throws IOException, InterruptedException {
if (content == null || content.length() == 0) {
return content; // Do nothing for an empty String
}
// // Expand environment variables
// String s = build.getEnvironment(listener).expand(content);
// Expand build variables
content = Util.replaceMacro(content, build.getBuildVariableResolver());

Jenkins jenkins = Jenkins.getInstance();
try {
ClassLoader uberClassLoader = Jenkins.getInstance().pluginManager.uberClassLoader;
// Expand environment variables
String body = run.getEnvironment(listener).expand(content);

if (jenkins.getPlugin("token-macro") != null) {
List macros = null;
// Expand build variables + token macro if they available
if (run instanceof AbstractBuild<?, ?>) {
final AbstractBuild<?, ?> build = (AbstractBuild<?, ?>) run;
body = Util.replaceMacro(body, build.getBuildVariableResolver());

//get private macroses like groovy template ${SCRIPT} if available
if (jenkins.getPlugin("email-ext") != null) {
Class<?> contentBuilderClazz = uberClassLoader.loadClass("hudson.plugins.emailext.plugins.ContentBuilder");
Method getPrivateMacrosMethod = contentBuilderClazz.getDeclaredMethod("getPrivateMacros");
macros = new ArrayList((Collection) getPrivateMacrosMethod.invoke(null));
}
try {
Jenkins jenkins = Jenkins.getActiveInstance();
ClassLoader uberClassLoader = jenkins.pluginManager.uberClassLoader;
List macros = null;
if (jenkins.getPlugin("token-macro") != null) {
// get private macroses like groovy template ${SCRIPT} if available
if (jenkins.getPlugin("email-ext") != null) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

😠 @lanwen tune to 4 nested at least?

Choose a reason for hiding this comment

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

MAJOR Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule

Copy link
Owner Author

Choose a reason for hiding this comment

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

When private macros will be in token macro nested if will disappear, but now it right logic. 3 is too small

Class<?> contentBuilderClazz = uberClassLoader.loadClass("hudson.plugins.emailext.plugins.ContentBuilder");
Method getPrivateMacrosMethod = contentBuilderClazz.getDeclaredMethod("getPrivateMacros");
macros = new ArrayList((Collection) getPrivateMacrosMethod.invoke(null));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the reason of such magic, but it causes much risks when the plugins get updated.
No strong opinion

Copy link
Owner Author

Choose a reason for hiding this comment

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

that existed code, just moved to if


//calls TokenMacro.expand(build, listener, content, false, macros)
Class<?> tokenMacroClazz = uberClassLoader.loadClass("org.jenkinsci.plugins.tokenmacro.TokenMacro");
Method tokenMacroExpand = tokenMacroClazz.getDeclaredMethod("expand", AbstractBuild.class,
TaskListener.class, String.class, boolean.class, List.class);
// call TokenMacro.expand(build, listener, content, false, macros)
Class<?> tokenMacroClazz = uberClassLoader.loadClass("org.jenkinsci.plugins.tokenmacro.TokenMacro");
Method tokenMacroExpand = tokenMacroClazz.getDeclaredMethod("expand", AbstractBuild.class,
TaskListener.class, String.class, boolean.class, List.class);

content = (String) tokenMacroExpand.invoke(null, build, listener, content, false, macros);
body = (String) tokenMacroExpand.invoke(null, build, listener, body, false, macros);
}
} catch (ClassNotFoundException e) {
LOGGER.error("Can't find class", e);
} catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
LOGGER.error("Can't evaluate macro", e);
}
} catch (ClassNotFoundException e) {
LOGGER.error("Can't find class", e);
} catch (NoSuchMethodException e) {
LOGGER.error("Can't evaluate macro", e);
} catch (InvocationTargetException e) {
LOGGER.error("Can't evaluate macro", e);
} catch (IllegalAccessException e) {
LOGGER.error("Can't evaluate macro", e);
}

return content;
return body;
}

public String getContent() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

import hudson.Util;
import hudson.console.AnnotatedLargeText;
import hudson.model.AbstractProject;
import hudson.model.Action;
import hudson.model.Job;
import org.apache.commons.jelly.XMLOutput;

import java.io.File;
Expand All @@ -16,13 +16,13 @@
* @author Alina Karpovich
*/
public class GitHubPRPollingLogAction implements Action {
private transient AbstractProject<?, ?> project;
private transient Job<?, ?> project;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or even item... I can imagine a use-case for Folders integration

Copy link
Owner Author

Choose a reason for hiding this comment

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

good idea, not sure only how test it on real example

Copy link
Contributor

Choose a reason for hiding this comment

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

Augh - the number of weird things that could fall under Item make my head explode!


public GitHubPRPollingLogAction(AbstractProject<?, ?> project) {
public GitHubPRPollingLogAction(Job<?, ?> project) {
this.project = project;
}

public AbstractProject<?,?> getOwner() {
public Job<?,?> getOwner() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

breaks binary compatibility. You need to bump the major version or to introduce bridge methods

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's alpha plugin!!!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, break away, I say.

return project;
}

Expand Down Expand Up @@ -50,8 +50,7 @@ public String getLog() throws IOException {
* @since 1.350
*/
public void writeLogTo(XMLOutput out) throws IOException {
new AnnotatedLargeText<>(getLogFile(),
Charset.defaultCharset(),true,this).writeHtmlTo(0,out.asWriter());
new AnnotatedLargeText<>(getLogFile(), Charset.defaultCharset(),true,this).writeHtmlTo(0,out.asWriter());
}

public File getLogFile() {
Expand Down
Loading