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

[JENKINS-26343] - Workflow Integration #15

Merged
merged 1 commit into from Aug 23, 2015

Conversation

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Apr 24, 2015

The change adds a Workflow step, which allows to publish HTML reports

  • - draft implementation
  • - merge with the master branch
  • - code/UI refactoring (prevent configs duplication)
  • - unit tests
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 24, 2015

https://jenkins.ci.cloudbees.com/job/plugins/job/htmlpublisher-plugin/16/console failed. The issue seems to be caused by Groovy conflicts

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 24, 2015

@mrooney, @jglick
A draft version is ready for the review

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Apr 24, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

<version>1.3</version>
</dependency>
</dependencies>
</plugin>

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

Is this necessary?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 24, 2015

Author Member

The default dependency is 1.5-jenkins-1. Groovy tests cannot be compiled with it. Short googling indicates a conflict with groovy-all.
http://stackoverflow.com/questions/9223689/solve-error-java-lang-nosuchmethoderror-org-codehaus-groovy-ast-modulenode-gets

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

Ah, I did not know there was a HtmlPublisherTest.groovy. Worth adding a TODO comment to this effect, since newer versions of the parent POM do use 1.5-jenkins-3.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 29, 2015

Author Member

Done

pom.xml Outdated
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
<version>1.0</version>

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

Recommend using the latest release compatible with 1.580.x, which would be 1.4.

ArrayList<String> aList = new ArrayList<String>();

try {
final InputStream is = this.getClass().getResourceAsStream(filePath);
final InputStream is = publisherClass.getResourceAsStream(filePath);

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

Overkill. Just hardcode HtmlPublisher.class. (The original code would have triggered a FindBugs violation, BTW; it is not safe for subclassing from another package.)

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 24, 2015

Author Member

I just investigated the code and decided that child classes may have other resources (e.g. Doxygen plugin could use HTML publisher with other header/footer). Changing of the code may cause regressions somewhere

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

child classes may have other resources (e.g. Doxygen plugin could use HTML publisher with other header/footer)

If this is intending to provide an API to other plugins, it should not accept a String resource path and use reflection to find that resource (as the current code does). Should instead accept a URL.

At any rate, GitHub search turns up no current external uses in @jenkinsci.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 24, 2015

Author Member

@mrooney, do you agree with replacing the call?

if (this.project instanceof AbstractProject) {
AbstractProject abstractProject = (AbstractProject) this.project;
if (this.project instanceof Job) {
Job jib = (Job) this.project;

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

Typo?

}

public class HTMLBuildAction extends BaseHTMLAction {
private final AbstractBuild<?, ?> build;
private final Run<?, ?> build;

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

BTW should make this transient and implement RunAction2.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 24, 2015

Author Member

In XML this thing appears to be , so it is not an issue. BTW, makes sense to change the behavior to prevent the entire build persistence on misuses.

// Add build action, if coverage is recorded for each build
if (this.keepAll) {
build.addAction(new HTMLBuildAction(build, this));
}
}

public Action getProjectAction(AbstractProject project) {
return new HTMLAction(project, this);
public Action getProjectAction(AbstractItem item) {

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

Not Job?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 24, 2015

Author Member

I just don't want to rework the code if somebody creates a new class with publishers based on AbstractItem. E.g. somebody may want to propagate reports to folders

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

Surely you would need to do a lot of refactoring to make that possible anyway?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 28, 2015

Author Member

Definitely :)
BTW, I see no problem in the usage of the Job's parent class till Actions begin depending on Job APIs (which we would like to avoid)

/**
* If true, will allow report to be missing and build will not fail on missing report.
*/
private final boolean allowMissing;

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

Could probably be dropped, since a script could itself check for a missing report and decide how to proceed.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 24, 2015

Author Member

I would leave it to retain the default behavior (fail on missing report)

This comment has been minimized.

Copy link
@jglick

jglick Apr 28, 2015

Member

Even if so, you could drop this field; a script wishing for equivalent functionality could just write

if (fileExists 'target/reports/x.html') {
    publishHtml reportDir: 'target/reports', …
}

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 24, 2015

Author Member

@jglick fileExists should support wildcards to get the similar behavior. I'd rather leave this thing as is.

private final boolean allowMissing;

@DataBoundConstructor
public PublishHTMLStep(String reportName, String reportDir, String reportFiles, boolean keepAll, boolean allowMissing) {

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

Recommend using @DataBoundSetter for any property with a sensible default.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 25, 2015

Author Member

Agreed. I use @DataBoundConstructor to keep fields final where it is possible. Does @DataBoundSetter allow to use defaults in Workflow scripts?

This comment has been minimized.

Copy link
@jglick

jglick Apr 28, 2015

Member

Does @DataBoundSetter allow to use defaults in Workflow scripts?

Yes. (You should also use the matching default in /lib/form/*.jelly controls.)

*/
@Restricted(DoNotUse.class)
@Extension
public class WorkflowActionsFactory extends TransientActionFactory<Job> {

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

You can probably delete this whole class and just make HtmlAction implement SimpleBuildStep.LastBuildAction, then delete HtmlPublisher.getProjectActions (or retain only the clause for MatrixProject).

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 24, 2015

Author Member

I've started from this approach, but reverted changes after getting into troubles with dependencies. SimpleBuildStep.LastBuildAction may present in SimpleBuildStep only, but the migration of the publisher to this would require a refactoring of all initial classes (HTMLPublisher target should become describable, etc.). Multi-entry selection in a single Workflow step is not a thing I would like to have there.

This comment has been minimized.

Copy link
@jglick

jglick Apr 28, 2015

Member

I was not suggesting making the publisher implement SimpleBuildStep. Only to use LastBuildAction instead of getProjectActions.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 28, 2015

Author Member

Agreed. I was also confused by nested interfaces and extensions. The latest core should work without SimpleBuildStep. However, the most of the code will be just migrated from the factory to the interface's method

<f:checkbox field="allowMissing" />
</td>
</tr>
</table>

This comment has been minimized.

Copy link
@jglick

jglick Apr 24, 2015

Member

Simpler to kill the table, which is not useful in the context in which step’s configuration would be displayed anyway.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 24, 2015

Author Member

The idea was to retain the initial design just to simplify the migration. BTW, the initial design lacks help popups, so we could use a common entries-based approach

This comment has been minimized.

Copy link
@jglick

jglick Apr 28, 2015

Member

Sure, fine for now.

@oleg-nenashev oleg-nenashev changed the title [JENKINS-26343] - Workflow Integration [WiP] [JENKINS-26343] - Workflow Integration Apr 24, 2015
return prepareRepos(build, build.getWorkspace(), launcher, listener, reportTargets, this.getClass());
}

public static boolean prepareRepos(Run<?, ?> build, FilePath workspace, Launcher launcher, TaskListener listener,

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 24, 2015

Author Member

Poor naming

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 25, 2015

Cc'e @reviewbybees, the work is in progress

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Apr 28, 2015

In XML this thing appears to be <build class="build" reference="../../.."/>, so it is not an issue.

In fact it is a serious issue. There are obscure cases involving lazy-loading and reloading of Runs in which such fields wind up storing complete copies of the parent, which upon deserialization then become phantom builds that throw NullPointerExceptions when used.


public HTMLBuildAction(AbstractBuild<?, ?> build, HtmlPublisherTarget actualHtmlPublisherTarget) {
public HTMLBuildAction(Run<?, ?> build, HtmlPublisherTarget actualHtmlPublisherTarget) {

This comment has been minimized.

Copy link
@jglick

jglick Apr 28, 2015

Member

Could drop the build parameter now that you implement RunAction2.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 28, 2015

Author Member

The interface is public. It makes sense to deprecate the method, but we still need to maintain the backward compatibility

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Apr 28, 2015

@jglick
It should be filed as a separate PR in any case. It is not a newly introduced bug. I'll do it

@gregsymons

This comment has been minimized.

Copy link

gregsymons commented Jun 26, 2015

Any idea when this work will be ready? I'm eagerly awaiting it :)

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jun 27, 2015

@gregsymons
I hope to continue working on this PR on the beginning of July.

pom.xml Outdated
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
<version>1.4</version>

This comment has been minimized.

Copy link
@amuniz

amuniz Jul 10, 2015

Member

Should this be optional?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 10, 2015

Author Member

Yes, it should

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 27, 2015

Author Member

done

This comment has been minimized.

Copy link
@jglick

jglick Jul 29, 2015

Member

🐜 workflow-step-api was expressly designed to avoid the need for optional dependencies: it contributes no UI or other runtime behavior on its own, it is just API.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jul 24, 2015

@mrooney I've returned to this activity

@oleg-nenashev oleg-nenashev self-assigned this Jul 24, 2015
@oleg-nenashev oleg-nenashev changed the title [WiP] [JENKINS-26343] - Workflow Integration [JENKINS-26343] - Workflow Integration Jul 27, 2015
@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Jul 27, 2015

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.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Jul 27, 2015

@reviewbybees @gregsymons
Reworked the implementation according to the new UI implementation in #19

@amuniz

This comment has been minimized.

Copy link
Member

amuniz commented Jul 27, 2015

🐝

super(actualHtmlPublisherTarget);
this.build = build;
}

public final AbstractBuild<?,?> getOwner() {
return build;
public final Run<?,?> getOwner() {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jul 29, 2015

Author Member

Binary compatibility issue

@amuniz

This comment has been minimized.

Copy link
Member

amuniz commented Aug 10, 2015

🐝

@amuniz

This comment has been minimized.

Copy link
Member

amuniz commented Aug 11, 2015

🐝 (again after the last change)

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 11, 2015

Thanks a lot for your efforts, @amuniz
@jglick , are you fine with the test suite?

* avoid confusions with actions referring to the data.
* @since TODO
*/
public class HTMLPublishedForProjectMarkerAction implements RunAction2 {

This comment has been minimized.

Copy link
@jglick

jglick Aug 11, 2015

Member

extends InvisibleAction would be easier.

This comment has been minimized.

Copy link
@jglick

jglick Aug 11, 2015

Member

Should be static.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Author Member

agreed

actions.add(report.getHTMLTarget().getProjectAction(j));
}

// If reports are being saved on the project level

This comment has been minimized.

Copy link
@jglick

jglick Aug 11, 2015

Member

I will confess to being lost at this point. Seems like there is some unnecessary complexity in this plugin. Normally all you really need, and what SimpleBuildStep.LastBuildAction offers, is a way to add some UI at the project level corresponding to what was published in the last build.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Author Member

I'll see if I can optimize it. "last action" for me is more preferable than "last successful actions", so I should try to migrate the code again

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

It is true that LastBuildAction should offer an option to select lastSuccessfulBuild (current behavior), lastStableBuild, or lastCompletedBuild, though I think lastSuccessfulBuild is the desired semantics for this plugin. (lastBuild is probably never a good option since the publisher will not in general have been run yet on that build.)

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 14, 2015

Author Member

If I understand correctly, now we don't switch to SimpleBuildStep.LastBuildAction. Is it correct?

This comment has been minimized.

Copy link
@jglick

jglick Aug 17, 2015

Member

Up to you. LastBuildAction currently works with lastSuccessfulBuild (i.e., stable or unstable). I think that is what this plugin happens to need, but it is not clear to me.


import javax.servlet.ServletException;
import jenkins.model.RunAction2;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

This comment has been minimized.

Copy link
@jglick

jglick Aug 11, 2015

Member

Unused imports I think.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Aug 11, 2015

🐝

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Aug 11, 2015

Actually make that a 🐛 for what seems to be a missing static that could cause problems in build.xml serialization.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 14, 2015

@jglick seems to be done

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Aug 17, 2015

🐝

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 17, 2015

@reviewbybees done
Addressed the minor comment from Jesse, which does not address the code. Squashing the commits now.

@oleg-nenashev oleg-nenashev force-pushed the oleg-nenashev:JENKINS-26343 branch from f632c7d to 75d92bb Aug 17, 2015
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 17, 2015

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 17, 2015

@mrooney Ready for the merge

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Aug 17, 2015

Now impossible to see what you changed since my bee…

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 18, 2015

@jglick
Before the squash I've added this comment

/**
+     * Constructor.
+     * @param target Target report to be published. May be null due if a user specifies an 
+     *               improper workflow (e.g. due to JENKINS-29711).
+     */
@mrooney

This comment has been minimized.

Copy link
Member

mrooney commented Aug 18, 2015

Thanks! This change is definitely above my understanding of Jenkins and Java, however it appears to be very well reviewed and thought out :) If you feel confident that it was well-tested manually for some basic configuration options, I'm happy to merge and release.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 18, 2015

@mrooney
Yes, it has passed manual tests + there are some automated tests for basic cases. I think it can be merged and released.

mrooney added a commit that referenced this pull request Aug 23, 2015
[JENKINS-26343] - Workflow Integration
@mrooney mrooney merged commit d26961f into jenkinsci:master Aug 23, 2015
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@mrooney

This comment has been minimized.

Copy link
Member

mrooney commented Aug 23, 2015

Released as 1.6, thanks for all the hard work and reviews!

@yannislionis

This comment has been minimized.

Copy link

yannislionis commented Dec 7, 2015

I've just tried using this with Jenkins ver. 1.609.14.1 (CloudBees Jenkins Enterprise 15.05), latest version of the plugin and I'm having issues. When I added:

step([$class: 'HtmlPublisher', directory: '**/target/serenity/site'])

I got:

java.lang.UnsupportedOperationException: no known implementation of interface jenkins.tasks.SimpleBuildStep is named HtmlPublisher

A freestyle build collects the reports successfully.

Any ideas?

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Dec 7, 2015

@yannislionis
Workflow integration for this plugin is implemented in a different way. The step is a separate class: https://github.com/jenkinsci/htmlpublisher-plugin/blob/master/src/main/java/htmlpublisher/workflow/PublishHTMLStep.java

I'm not sure why you want to use the "step" instead of the integrated step available for this plugin (see scriptexamples in the test classes)

@yannislionis

This comment has been minimized.

Copy link

yannislionis commented Dec 7, 2015

Thanks @oleg-nenashev. The main issue is that, as a newcomer to workflow, I wasn't sure how to use this and the line above was my attempt. What I was missing was a "here's how to use it" example. I'm using the test you pointed me to to work it out now, but a documented example would be better - unless it's somewhere and I've missed it...

@p14n

This comment has been minimized.

Copy link

p14n commented Mar 3, 2016

For future peeps stumbling across this, he meant this (I assume):

publishHTML(target: [allowMissing: " + target.getAllowMissing() +
", keepAll: " + target.getKeepAll() + ", reportDir: '" + target.getReportDir() +
"', reportFiles: '" + target.getReportFiles() + "', reportName: '" + target.getReportName() + "']) \n"
+ "}

Ah, maybe not.

java.lang.NoSuchMethodError: No such DSL method 'publishHTML' found among [archive, bat, build, catchError, checkout, deleteDir, dir, echo, error, fileExists, git, input, isUnix, load, mail, node, parallel, properties, pwd, readFile, retry, sh, sleep, stage, stash, step, svn, timeout, tool, unarchive, unstash, waitUntil, withEnv, wrap, writeFile, ws]

Who knows.

@amuniz

This comment has been minimized.

Copy link
Member

amuniz commented Mar 3, 2016

@p14n yes, he meant that. Did you have htmlpublisher plugin installed? Given the NoSuchMethodError I'd say no.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Mar 3, 2016

Sorry for the vague messages. Just ping me if I don't reply

@p14n

This comment has been minimized.

Copy link

p14n commented Mar 3, 2016

@amuniz felt silly when I realised. Thanks.
@oleg-nenashev np, thanks for the plugin :)

My final example:
publishHTML(target: [allowMissing: false, keepAll: true, reportDir: 'target/cucumber-html-reports/',
reportFiles: 'feature-overview.html', reportName: 'Test results'])

@abeeskau

This comment has been minimized.

Copy link

abeeskau commented Sep 8, 2017

@oleg-nenashev thank you for your work on this plugin!
I am trying to use it from a global shared library. In my var/runFunctionalTests.groovy I have code that passes the DSL steps instance to a class (execute method):

import org.magento.ci.tests.Functional

def call() {
    node {
        def functionalTest = new Functional('Functional')
        buildImages(functionalTest)
        parallel functionalTest.execute(steps, label_slave)
    }
}

/src/org/magento/ci/tests/Functional.groovy snippet:

def execute(DSL steps, String agentLabel) {
    steps.node(agentLabel) {
        steps.stage('Tests') {
            //snip code
            steps.step([$class: 'PublishHTMLStep', allowMissing: false, alwaysLinkToLastBuild: false, keepAll: false, reportDir: 'results', reportFiles: 'report.html', reportName: 'HTML Report', reportTitles: ''])
            steps.step([$class: 'ArtifactArchiver', artifacts: 'results/*, *-results.tar.gz', excludes: null])
            steps.step([$class: 'JUnitResultArchiver', allowEmptyResults: true, testResults: 'phpunit.xml'])
        }
    }
}

This syntax does not work. I get a strange syntax error near unexpected token `)' error.
Also tried without steps.step reference and got hudson.remoting.ProxyException: groovy.lang.MissingMethodException: No signature of method: org.magento.ci.tests.Functional.publishHTML() is applicable for argument types: (java.util.LinkedHashMap) values: [[allowMissing:false, alwaysLinkToLastBuild:false, keepAll:false, ...]]
Referencing ArtifactArchiver and JUnitResultArchiver classes works fine.
Any suggestions would be greatly appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.