[JENKINS-27208] Compatible with Workflow #10

recena · 2015-09-27T21:22:12Z

node {
  checkout changelog: false, poll: false, scm: [$class: 'SubversionSCM', additionalCredentials: [], excludedCommitMessages: '', excludedRegions: '', excludedRevprop: '', excludedUsers: '', filterChangelog: false, ignoreDirPropChanges: false, includedRegions: '', locations: [[credentialsId: '45f065ab-6e41-40d2-9f01-db726aa4621d', depthOption: 'infinity', ignoreExternalsOption: true, local: '.', remote: 'https://subversion.assembla.com/svn/jenkins-26318/trunk']], workspaceUpdater: [$class: 'UpdateUpdater']]
  sh 'mvn clean install -Dmaven.test.skip'
  step([$class: 'LogParserPublisher', 'useProjectRule': true, 'projectRulePath': 'logparser-rules.txt'])
}

@reviewbybees, specially @jglick and @amuniz

ghost · 2015-09-27T21:23:10Z

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 · 2015-09-27T21:29:24Z

src/main/java/hudson/plugins/logparser/LogParserAction.java

@@ -59,7 +64,7 @@ public static String getUrlNameStat() {
        return urlName;
    }

-    public AbstractBuild<?, ?> getOwner() {
+    public Run<?, ?> getOwner() {


Binary compatibility issue

@oleg-nenashev I'd like to talk wih the maintainer about it. The last release of this plugin was on 15 Dec 2010. Do you think make sense?

I'm pretty sure I've used these actions in my scripts at the previous work... And yes, the plugin used to work well :)

BTW, a release with a new major version could be an option. I have no idea how to properly test the previously merged commits since the last release

There's an annotation that can be used. It's called @WithBridgeMethods or something.

Yeah, example here.

oleg-nenashev · 2015-09-27T22:08:22Z

src/main/java/hudson/plugins/logparser/LogParserParser.java

@@ -40,7 +41,7 @@

    public LogParserParser(final FilePath parsingRulesFile,
            final boolean preformattedHtml, final VirtualChannel channel)
-            throws IOException {
+            throws IOException, InterruptedException {


Breaks the compatibility

Actually, not sure why it was added given that the constructor code didn't change.

jenkinsadmin · 2015-09-28T00:01:55Z

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

tfennelly · 2015-09-28T07:15:44Z

pom.xml

  </parent>
  <artifactId>log-parser</artifactId>
  <packaging>hpi</packaging>
  <version>1.1-SNAPSHOT</version>
  <name>Log Parser Plugin</name>
+
+  <properties>
+    <workflow-jenkins-plugin.version>1.4</workflow-jenkins-plugin.version>


Seeing as the version is only ref'd in one place, is it really worth splitting out a property for it?

I agree. I decided to add this section properties because I'm working on #9 and I'll need to reference more values.

I think you can use 1.7 as you upgraded core to 1.596.1

amuniz · 2015-09-28T08:24:08Z

🐝 despite binary incompatibilities. You have to decide (with the maintainer, if still active) if you want to keep backward compatibility. IMO it's a common rule in plugins and it's a good option -- if possible -- (and in this case is easy with @WithBridgeMethods and a couple of try-catch-throw).

recena · 2015-09-28T08:35:22Z

@amuniz Exactly. If the maintainer wants to keep backward compatibility, I'll do it.

tfennelly · 2015-09-28T09:20:50Z

🐝

recena · 2015-09-28T09:30:16Z

@tfennelly @amuniz @oleg-nenashev Thanks so much for your time.

Before to consider this PR done, I'm would like to contact with the maintainer and verify snippet generator is working fine.

…xception

recena · 2015-09-30T11:26:46Z

@tfennelly @amuniz @oleg-nenashev

I had to do some changes:

Snippet generator did not work as I expected.
Add a specific test for workflow compatibility

Could you review this PR again? Thank in advance.

tfennelly · 2015-09-30T11:36:25Z

🐝 LGTM

amuniz · 2015-09-30T17:18:15Z

src/main/resources/hudson/plugins/logparser/LogParserPublisher/config.jelly

@@ -9,7 +9,7 @@
  <f:entry title="Show log parser graphs" help="/plugin/log-parser/parser_graphs.html">
    <f:checkbox name="log-parser.showGraphs" checked="${instance.showGraphs}"/>
  </f:entry>
-  <f:radioBlock title="Use global rule" name="log-parser.useProjectRule" value="false" checked="${instance.useProjectRule!=true}">
+  <f:radioBlock title="Use global rule" name="log-parser.useProjectRule" value="false" checked="${instance.useProjectRule!=true}" inline="true">


Did you check if there some acceptance test relying on this? The inline attribute changes the selector to access the field in acceptance tests.

@amuniz It seems there aren't acceptance tests for this plugin.

amuniz · 2015-09-30T17:23:12Z

Kudos for adding a test. A configuration round trip test would be in place as well, since there were some changes in data bound aware fields. At any rate 🐝

recena · 2015-09-30T20:26:23Z

@amuniz Nice review. I'll address your comments.

recena · 2015-10-01T17:54:30Z

@reviewbybees done

recena · 2015-10-01T17:55:41Z

@jborghi Consider this PR ready to merge. Your comments are welcome.

oleg-nenashev · 2015-10-03T21:14:57Z

pom.xml

+      <groupId>org.jenkins-ci.plugins.workflow</groupId>
+      <artifactId>workflow-aggregator</artifactId>
+      <version>${workflow-jenkins-plugin.version}</version>
+     <scope>test</scope>


NIT: indenting

recena · 2015-10-05T10:17:44Z

@reviewbybees done

jglick · 2015-10-05T21:51:27Z

src/main/java/hudson/plugins/logparser/LogParserParser.java

-            final String logFileLocation, final int linesInLog,
-            final Logger logger) throws IOException, InterruptedException {
+    @Deprecated
+    private void parseLogBody(final AbstractBuild build, final BufferedWriter writer, final FilePath filePath, final


Deprecating a private method makes no sense. You can just delete it.

@jglick It is true. Done.

jglick · 2015-10-05T21:59:30Z

src/main/java/hudson/plugins/logparser/LogParserPublisher.java

+    }
+
+    @Override
+    public void perform(Run<?, ?> run, FilePath workspace, Launcher launcher, TaskListener listener) throws


BTW naming the first parameter build would minimize diff lines.

@jglick Done

jglick · 2015-10-05T22:06:48Z

belated 🐝

recena · 2015-10-05T22:12:01Z

@jglick your 🐝 are never belated. Thank so much!

tfennelly · 2015-10-06T07:29:38Z

🐝

recena · 2015-10-06T08:55:37Z

@reviewbybees done

ghost · 2015-10-06T08:57:34Z

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

[JENKINS-27208] Compatible with Workflow

recena · 2015-10-14T18:35:44Z

@orrc My commit message was confused. The obsolete part was related to newInstance() and how the params are processed. Only that.

[JENKINS-27208] Initial source code modifications

7847264

recena changed the title ~~[JENKINS-27208] Compatible with Workflow~~ [WiP] [JENKINS-27208] Compatible with Workflow Sep 27, 2015

recena mentioned this pull request Sep 27, 2015

[JENKINS-29308] Update project configuration #9

Merged

4 tasks

oleg-nenashev reviewed Sep 27, 2015
View reviewed changes

[JENKINS-27208] DataBoundConstructor, DataBoundSetter

2124750

oleg-nenashev reviewed Sep 27, 2015
View reviewed changes

tfennelly reviewed Sep 28, 2015
View reviewed changes

recena mentioned this pull request Sep 29, 2015

Fixed tests for workflow recena/log-parser-plugin#2

Open

recena and others added 4 commits September 29, 2015 23:39

[JENKINS-27208] Removed obsolete way to work

e59455c

[JENKINS-27208] InterruptedException is catched and relaunched as IOE…

cc2c42e

…xception

[JENKINS-27208] Updated Workflow dependency

9f8e6b8

[JENKINS-27208] Added a test specific for workflow compatibility

95a68a0

recena changed the title ~~[WiP] [JENKINS-27208] Compatible with Workflow~~ [JENKINS-27208] Compatible with Workflow Sep 30, 2015

amuniz reviewed Sep 30, 2015
View reviewed changes

[JENKINS-27208] @amuniz's comments were addressed

8792299

oleg-nenashev reviewed Oct 3, 2015
View reviewed changes

[JENKINS-27208] Wrong indent

ebeb53e

jglick reviewed Oct 5, 2015
View reviewed changes

[JENKINS-27208] Removed an unnecessary method

b1a2956

jglick reviewed Oct 5, 2015
View reviewed changes

[JENKINS-27208] A variable has been renamed to reduce the changes (diff)

4fcb6fb

recena added a commit that referenced this pull request Oct 13, 2015

Merge pull request #10 from recena/JENKINS-27208

7cce021

[JENKINS-27208] Compatible with Workflow

recena merged commit 7cce021 into jenkinsci:master Oct 13, 2015

recena deleted the JENKINS-27208 branch October 13, 2015 13:49

recena mentioned this pull request Jan 13, 2016

[JENKINS-27206] - Move claim-plugin to use SimpleBuildStep jenkinsci/claim-plugin#18

Merged

[JENKINS-27208] Compatible with Workflow #10

[JENKINS-27208] Compatible with Workflow #10

Conversation

recena commented Sep 27, 2015

ghost commented Sep 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenkinsadmin commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuniz commented Sep 28, 2015

recena commented Sep 28, 2015

tfennelly commented Sep 28, 2015

recena commented Sep 28, 2015

recena commented Sep 30, 2015

tfennelly commented Sep 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuniz commented Sep 30, 2015

recena commented Sep 30, 2015

recena commented Oct 1, 2015

recena commented Oct 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

recena commented Oct 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Oct 5, 2015

recena commented Oct 5, 2015

tfennelly commented Oct 6, 2015

recena commented Oct 6, 2015

ghost commented Oct 6, 2015

recena commented Oct 14, 2015