-
Notifications
You must be signed in to change notification settings - Fork 36
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-34983] - Add pipeline support #12
Conversation
It's unclear to me who should review/merge/release this. I note that there were previous pull requests to add Pipeline support to this plugin in 2016 and 2017. Maybe third time's a charm? Here's hoping for 2018. Paging @jglick since he was the last person to release an update of this plugin. |
Nice work! Thanks for the improvements. Can you explain in more detail why Pipeline support should be added to the text finder plugin when logfile reading is already available from existing Pipeline plugins? Refer to the logContains usage and the assertContains implementation which uses the manager object from the Groovy post build plugin. Are you unwilling to use the Groovy post build plugin, or is there some other reason? |
The code looks good to me. You've done a nice job of documenting your changes and improving things. I second @MarkEWaite's question about the logContains/assertContains. Do those not do what you need? |
You're welcome!
Pipeline support should be added to Text Finder to enable a smooth transition from Freestyle to Pipeline for legacy jobs without incurring the additional risk of regressions due to a change in the underlying implementation of the job as a result of using a different plugin. The Text Finder plugin also has the advantage of working for jobs that don't have the Groovy Sandbox enabled. While brand new Pipeline jobs are probably already following best practices and using the sandbox, this may not be the case for legacy jobs. As such, adding Pipeline support to this plugin would be convenient for users who want to migrate legacy code to Pipeline with as few changes as possible to the underlying job configuration and implementation. |
As a specific comment to:
I'm not sure I understand what you mean by that. A blog post from Tyler Croy strongly recommends to not disable the Groovy Sandbox. I don't disable the Groovy Sandbox and I use the Groovy Post Build Plugin throughout my Jenkins installation. I thought the default is that the Groovy Sandbox is enabled and an administrator must make a conscious (and dangerous) choice to disable it. |
The administrator may want to consciously disable the Grooxy Sandbox and still use a plugin to analyze the build logs. The Text Finder plugin supports this use case, while the Groovy post build plugin does not. Hence this is another reason to port Text Finder to Pipeline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TaskListener serialization over Remoting is something I would recommend to avoid (in the context of JEP-200 & Co). I would recommend to use RemoteOutputStream and to create some auto tests which verify the behavior on agents
The rest of the code looks great.
pom.xml
Outdated
@@ -4,14 +4,19 @@ | |||
<parent> | |||
<groupId>org.jenkins-ci.plugins</groupId> | |||
<artifactId>plugin</artifactId> | |||
<version>1.480</version> | |||
<version>3.4</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.14 now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, 3.15 now ;-)
@@ -201,8 +215,10 @@ private Pattern compilePattern(PrintStream logger) { | |||
return pattern; | |||
} | |||
|
|||
@Symbol("textFinder") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe findText()
to make it look like method. No strong preference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the symbol to findText
as you requested.
@@ -233,6 +250,61 @@ public FormValidation doCheckRegexp(@QueryParameter String value) throws IOExcep | |||
} | |||
} | |||
|
|||
private static class FileChecker extends MasterToSlaveFileCallable<Boolean> { | |||
|
|||
private final TaskListener listener; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend to pass RemoteOutputStream
there.
TaskListener
gets here from API, and there is no guarantee that it will be always serializable over Remoting if somebody uses API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed {{TaskListener}} to {{RemoteOutputStream}} as you requested.
I created automated tests that verify the behavior on an agent as you requested. |
👍 |
Jenkinsfile
Outdated
@@ -0,0 +1 @@ | |||
buildPlugin() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also set up testing against the 1.121.x branch so that we discover compatibility issues
buildPlugin(jenkinsVersions: [null, '2.121.1'])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@jglick Do you still maintain the plugin? If yes, would you be able to spin the release? If no, would you be fine if it is marked for adoption? |
No. Apparently I cut a release four and a half years ago, of which I have no memory.
Go ahead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid I lack time to review miscellaneous conversions of plugins to Pipeline compatibility.
On a general note, I doubt you really need Pipeline compatibility for it, or that you need a plugin like this at all—just include something in your main build tool to the effect of
if fgrep -rl TODO .
then
echo 'Problems found! ^^^'
exit 1
fi
You can also use a tiny bit of Groovy to set builds to unstable rather than failed, etc.
Oh, I agree. I don't think anyone who is writing a brand new Pipeline job would use this plugin. As I wrote above, my main use case is in performing a mechanical conversion of a set of legacy jobs with as few changes as possible. Once my legacy jobs are migrated to Pipeline, I'll start rewriting significant parts of them to use conventions like the one described above. |
This is https://issues.jenkins-ci.org/browse/JENKINS-34983 BTW @basil Would you be interested to take ownership of the plugin? I have unassigned @jglick according to the discussion and marked the plugin for adoption, so the ownership can be transferred at any moment |
Sure, I'll take it. There aren't too many open bugs or pull requests, anyway. |
@basil great, granted you permissions. Welcome aboard! |
@basil Will there be a release soon? Interested in using this plugin in my project :). Also, it would be nice to see an example for pipeline usage in the README. Thank you! |
Problem
The Text Finder plugin doesn't support Pipeline. It also has an old parent POM.
Solution
Add Pipeline support and update the parent POM to the latest version.
Implementation
I started by updating the parent POM to the latest version and specifying a minimum Jenkins version that had decent Pipeline support. I also added
structs
to the dependency list (for using@Symbol
) and added the Pipeline plugin dependencies required for testing. Updating to the latest parent POM necessitated adding<?jelly escape-by-default='true'?>
to the.jelly
files.To add Pipeline support to the plugin, I made it implement
SimpleBuildStep
as recommended in the documentation. As suggested in the "Constructor vs. setters" section of the documentation, I replaced the lengthy existing@DataBoundConstructor
with a short one taking just truly mandatory parameters. For all optional parameters, I created a public setter marked with@DataBoundSetter
(with any non-null default value set in the constructor or field initializer). This allows most parameters to be left at their default values in a Pipeline script, not to mention simplifying ongoing code maintenance because it is much easier to introduce new options this way. For Java-level compatibility, I left the previous constructor in place, but marked it@Deprecated
. I also removed@DataBoundConstructor
from it (there can be only one). Finally, I added atextFinder
symbol (with@Symbol
) to make use of this plugin from Pipeline more elegant.Bumping the minimum Jenkins version resulted in a compilation error in
FileCallable
, so I replaced this with a call to the newerMasterToSlaveFileCallable
class. I also extracted this callable into a static class to prevent warnings about Remoting serialization of anonymous inner classes. This necessitated marking a few methods called by theCallable
as static methods.Testing
This plugin had no tests, so I wrote some tests for both Freestyle and Pipeline jobs. The tests get about 60% coverage of the plugin. I got coverage of all the positive code paths and some (but not all) of the error conditions.