JENKINS-24207: Add new selector for Perforce/P4 that allows to update JIRA issues based on jobs #95

J-cztery · 2016-04-27T16:28:39Z

This change is

coveralls · 2016-04-27T16:57:57Z

Coverage increased (+1.5%) to 46.186% when pulling eb096c9 on J-cztery:AddJobSupport into e23c778 on jenkinsci:master.

rantoniuk · 2016-04-28T02:33:56Z

Nice! Can I ask you to create JIRA ticket for this in issues.jenkins-ci.org and reference that JIRA number in the commit message? That helps in the release notes :)

rantoniuk · 2016-04-28T02:35:15Z

src/main/java/hudson/plugins/jira/selector/p4/JobIssueSelector.java

+
+    @Override
+    public Set<String> findIssueIds(Run<?, ?> run, JiraSite site, TaskListener listener) {
+        // TODO Auto-generated method stub


If this is not implemented yet, do we need to have this class in the commit?

No, definitely not. I just wanted to have your opinion on the direction. It got late last night so i had to stop. I will either implement p4 plugin or remove dependencies on it and the selector.

Great, sounds good, thanks!

coveralls · 2016-04-28T15:47:35Z

Coverage increased (+2.5%) to 47.211% when pulling 561926e on J-cztery:AddJobSupport into e23c778 on jenkinsci:master.

J-cztery · 2016-04-28T15:51:39Z

There already was a JIRA issue. Let me know if you have any other comments. I will squash the commits once it is final.

coveralls · 2016-04-29T03:21:20Z

Coverage increased (+2.5%) to 47.211% when pulling 3da6d47 on J-cztery:AddJobSupport into e23c778 on jenkinsci:master.

coveralls · 2016-04-29T03:23:37Z

Coverage increased (+2.5%) to 47.211% when pulling 3da6d47 on J-cztery:AddJobSupport into e23c778 on jenkinsci:master.

rantoniuk · 2016-04-30T12:49:15Z

There's one thing I don't like - the static methods in DefaultIssueSelector and using them in the JobIssueSelector in that way.. Maybe you could move them AbstractIssueSelector and make non-static?

rantoniuk · 2016-04-30T15:01:10Z

src/main/java/hudson/plugins/jira/selector/jobs/JobIssueSelector.java

+import hudson.plugins.jira.RunScmChangeExtractor;
+import hudson.plugins.jira.selector.AbstractIssueSelector;
+import hudson.plugins.jira.selector.DefaultIssueSelector;
+


Please also add JavaDoc describing this class

J-cztery · 2016-05-01T11:41:39Z

@warden AbstractIssueSelector is not a good place for this methods. But I see what you are coming from. Please have a look at the changes i made (I think they are what you wanted but they are in DefaultIssueSelector rather than in AbstractIssueSelector) and see if you like them or not.
The idea is that DefaultIssueSelector contains basic logic, i.e. take issues from parameters, carried over, dependent builds and from current builds. Any piece of this behaviour can be overridden.

Added javadoc.

Added config.jelly resources for selectors, including DefaultIssueSelector.

Strings are now taken from properties..

coveralls · 2016-05-01T11:50:04Z

Coverage increased (+2.8%) to 47.509% when pulling eac40ba on J-cztery:AddJobSupport into e23c778 on jenkinsci:master.

rantoniuk · 2016-05-01T11:57:45Z

src/main/java/hudson/plugins/jira/selector/DefaultIssueSelector.java

-     *            
-     */
-    protected static void findIssues(Run<?, ?> build, Set<String> ids, Pattern pattern, TaskListener listener) {
+    protected static void findIssues(Run<?, ?> build, Set<String> issueIds, Pattern pattern, TaskListener listener) {


this static should not be needed here anymore :)

If i remove it, i will break binary compatibility...
Also, tests are using it... Tests can be reworked to not use this. But binary compatibility might be a bigger issue.

True, that's a valid point, lets keep it as is then for now.

…low to update JIRA issues based on jobs associated with changes. Other changes: 1. Refactor selectors. Break down methods in DefaultIssuesSelector so that it is easier to reuse. Side effect is that DefaultIssueSelector now will be fully recursive (once the other problems are fixed). 2. Fix resources (DefaultUpdaterIssueSelector/config.jelly->selectors/DefaultIssueSelector/config.jelly).

coveralls · 2016-05-01T14:18:17Z

Coverage increased (+2.8%) to 47.509% when pulling 2eace06 on J-cztery:AddJobSupport into e23c778 on jenkinsci:master.

rantoniuk · 2016-05-01T17:06:25Z

src/main/java/hudson/plugins/jira/selector/perforce/PerforceJobIssueSelector.java

+import hudson.scm.ChangeLogSet;
+import hudson.scm.ChangeLogSet.Entry;
+
+@Extension(optional = true)


Double annotation, one is already at DescriptorImpl.

I am not sure what you are referring to. These are two different classes. Each of them is marked as an extension. Feel free to modify that code if you do not like it.
I do not have any more time to test if @extension annotates nested classes automatically and documenation does not state it clearly.

Per documentation:

* Marks a field, a method, or a class for automatic discovery, so that Hudson can locate * implementations of {@link ExtensionPoint}s automatically. * * <p> * (In contrast, in earlier Hudson, the registration was manual.) * * <p> * In a simplest case, put this on your class, and Hudson will create an instance of it * and register it to the appropriate {@link ExtensionList}. * * <p> * If you'd like Hudson to call * a factory method instead of a constructor, put this annotation on your static factory * method. Hudson will invoke it and if the method returns a non-null instance, it'll be * registered. The return type of the method is used to determine which {@link ExtensionList} * will get the instance. * * Finally, you can put this annotation on a static field if the field contains a reference * to an instance that you'd like to register. *

See DefaultIssueSelector where @extension is only at DescriptorImpl level.

rantoniuk · 2016-05-02T08:00:16Z

So I guess after removing the doubled annotations I'm okay with this one 👍

J-cztery · 2016-05-02T10:26:34Z

@warden this is how much i can afford to contribute my work to this change. At this time it is either take it or leave it. I am sure there are others willing to contribute the changes you require.
Thank you for your useful comments and proposals!

* upstream/pr/95: JENKINS-24207 - Add new selectors for Perforce and P4 plugins that allow to update JIRA issues based on jobs associated with changes. Other changes: 1. Refactor selectors. Break down methods in DefaultIssuesSelector so that it is easier to reuse. Side effect is that DefaultIssueSelector now will be fully recursive (once the other problems are fixed). 2. Fix resources (DefaultUpdaterIssueSelector/config.jelly->selectors/DefaultIssueSelector/config.jelly).

rantoniuk reviewed Apr 28, 2016
View reviewed changes

J-cztery changed the title ~~Add new selector for Perforce that allows to update JIRA issues based on jobs~~ JENKINS-24207: Add new selector for Perforce that allows to update JIRA issues based on jobs Apr 28, 2016

J-cztery force-pushed the AddJobSupport branch 2 times, most recently from c108b14 to 3da6d47 Compare April 29, 2016 03:15

rantoniuk reviewed Apr 30, 2016
View reviewed changes

J-cztery force-pushed the AddJobSupport branch from 3da6d47 to eac40ba Compare May 1, 2016 11:35

J-cztery changed the title ~~JENKINS-24207: Add new selector for Perforce that allows to update JIRA issues based on jobs~~ JENKINS-24207: Add new selector for Perforce/P4 that allows to update JIRA issues based on jobs May 1, 2016

rantoniuk reviewed May 1, 2016
View reviewed changes

J-cztery force-pushed the AddJobSupport branch from eac40ba to 2eace06 Compare May 1, 2016 14:08

J-cztery mentioned this pull request May 1, 2016

Fix getting dependent issues #97

Merged

rantoniuk reviewed May 1, 2016
View reviewed changes

rantoniuk merged commit 2eace06 into jenkinsci:master May 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JENKINS-24207: Add new selector for Perforce/P4 that allows to update JIRA issues based on jobs #95

JENKINS-24207: Add new selector for Perforce/P4 that allows to update JIRA issues based on jobs #95

J-cztery commented Apr 27, 2016 •

edited by rantoniuk

coveralls commented Apr 27, 2016

rantoniuk commented Apr 28, 2016 •

edited

rantoniuk Apr 28, 2016 •

edited

J-cztery Apr 28, 2016

rantoniuk Apr 28, 2016

coveralls commented Apr 28, 2016

J-cztery commented Apr 28, 2016

coveralls commented Apr 29, 2016

coveralls commented Apr 29, 2016

rantoniuk commented Apr 30, 2016

rantoniuk Apr 30, 2016

J-cztery commented May 1, 2016

coveralls commented May 1, 2016

rantoniuk May 1, 2016

J-cztery May 1, 2016

rantoniuk May 1, 2016

coveralls commented May 1, 2016

rantoniuk May 1, 2016

J-cztery May 2, 2016

rantoniuk May 2, 2016

rantoniuk commented May 2, 2016

J-cztery commented May 2, 2016

JENKINS-24207: Add new selector for Perforce/P4 that allows to update JIRA issues based on jobs #95

JENKINS-24207: Add new selector for Perforce/P4 that allows to update JIRA issues based on jobs #95

Conversation

J-cztery commented Apr 27, 2016 • edited by rantoniuk

coveralls commented Apr 27, 2016

rantoniuk commented Apr 28, 2016 • edited

rantoniuk Apr 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 28, 2016

J-cztery commented Apr 28, 2016

coveralls commented Apr 29, 2016

coveralls commented Apr 29, 2016

rantoniuk commented Apr 30, 2016

Choose a reason for hiding this comment

J-cztery commented May 1, 2016

coveralls commented May 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rantoniuk commented May 2, 2016

J-cztery commented May 2, 2016

J-cztery commented Apr 27, 2016 •

edited by rantoniuk

rantoniuk commented Apr 28, 2016 •

edited

rantoniuk Apr 28, 2016 •

edited