-
Notifications
You must be signed in to change notification settings - Fork 273
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-26010] Workflow compatibility #240
Changes from 7 commits
07075f4
a56938a
813c290
6e87548
fb4d4d8
53d52e3
e67379d
024782a
f84c610
ddb0822
2031da3
e8fb882
600865b
e431ff8
536e261
992bddf
894cd47
1544cc6
cccb9e8
b46b101
4afc8df
a12c41d
64ec1c9
ee96231
4fe3786
eb4a814
4af752b
ac68390
970e457
7a4b484
0b8ff13
8ef9a19
0982b25
78faddc
ecc8dca
93a64bc
0c7f281
5321e90
24b63ce
1e76ce1
7caf9fc
7c6e5aa
9d5ac2a
3dd663b
0a19c16
4ff2009
98a9148
00d41b6
c3ce41f
f55bd54
c4f8562
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,7 @@ | |
import com.sonymobile.tools.gerrit.gerritevents.dto.attr.Provider; | ||
import com.sonymobile.tools.gerrit.gerritevents.dto.events.GerritTriggeredEvent; | ||
import hudson.Plugin; | ||
import hudson.model.AbstractProject; | ||
import hudson.model.Job; | ||
import hudson.model.Api; | ||
import hudson.model.Hudson; | ||
import hudson.model.Items; | ||
|
@@ -96,7 +96,7 @@ public class PluginImpl extends Plugin { | |
public static final Permission RETRIGGER = new Permission(PERMISSION_GROUP, | ||
"Retrigger", | ||
Messages._RetriggerPermissionDescription(), | ||
AbstractProject.BUILD); | ||
Job.BUILD); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW this is defined in |
||
|
||
private static final Logger logger = LoggerFactory.getLogger(PluginImpl.class); | ||
private final List<GerritServer> servers = new CopyOnWriteArrayList<GerritServer>(); | ||
|
@@ -436,10 +436,10 @@ public static GerritHandler getHandler_() { | |
* @param serverName the name of the Gerrit server. | ||
* @return the list of jobs configured with this server. | ||
*/ | ||
public List<AbstractProject> getConfiguredJobs(String serverName) { | ||
LinkedList<AbstractProject> configuredJobs = new LinkedList<AbstractProject>(); | ||
for (AbstractProject<?, ?> project : Hudson.getInstance().getItems(AbstractProject.class)) { //get the jobs | ||
GerritTrigger gerritTrigger = project.getTrigger(GerritTrigger.class); | ||
public List<Job> getConfiguredJobs(String serverName) { | ||
LinkedList<Job> configuredJobs = new LinkedList<Job>(); | ||
for (Job<?, ?> project : Hudson.getInstance().getItems(Job.class)) { //get the jobs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inherital sin: change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inherital sin: change |
||
GerritTrigger gerritTrigger = GerritTrigger.getTrigger(project); | ||
|
||
//if the job has a gerrit trigger, check whether the trigger has selected this server: | ||
if (gerritTrigger != null && gerritTrigger.getServerName().equals(serverName)) { | ||
|
@@ -458,7 +458,7 @@ public List<AbstractProject> getConfiguredJobs(String serverName) { | |
*/ | ||
@Nonnull | ||
//CS IGNORE MethodName FOR NEXT 1 LINES. REASON: Static equivalent marker. | ||
public static List<AbstractProject> getConfiguredJobs_(String serverName) { | ||
public static List<Job> getConfiguredJobs_(String serverName) { | ||
PluginImpl plugin = getInstance(); | ||
if (plugin == null) { | ||
logger.debug("Error, plugin instance could not be found!"); | ||
|
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.
What would happen if it's an instance of
WorkflowJob
?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.
Does WorkflowJob have a way to add and remove triggers? I assume we'd want something similar to that? I know that the ParameterizedJobMixin does not have support for add triggers, but maybe we can just instanceOf to WorkflowJob here?
According to the source for WorkflowJob.java, it appears that WorkflowJob has an "addTrigger" function and getTriggers function, but this isn't exposed by the Mixin API. It would feel really awkward to instanceof and then use the same code both times.... Also, it doesn't support "getTrigger" standalone only the getTriggers...
Kind of ugly either way.
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.
The
getTriggers()
method is defined inParameterizedJobMixIn.ParameterizedJob
. We can cast to this one better thanWorkflowJob
.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.
@amuniz At the moment, nothing will happen for a Workflow Job i.e. add/remove of triggers is not supported for workflow as things stand. That's why I marked it with a
TODO
. We need to find a way of handling it for workflow.AbstractProject
has methods for adding and removing triggers, but that's no help for a Workflow job.Trigger
rename
might be fixable by finding the current trigger and renaming the server on it Vs removing and readding the trigger (at the moment,rename
is supported by removing the trigger and adding a new one). Not sure yet howremove
will be supported (when a Gerrit server is removed we need to find all Jobs with triggers for that server and remove them).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 add a trigger easily. It seems like the trigger add code will remove the old trigger already. Sadly WorkflowJob.java does not export a remove trigger function like the code does. Maybe we just don't bother removing the old trigger at all when a server is removed?
What's the reason that we actually have remove/add trigger code here specifically? I presume to prevent a bug? What do other trigger plugins do to prevent this sort of issue?
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.
Auto removing zombie triggers after a Gerrit server is removed seems to make sense to me.
In any case (and imo), the debate on the pros and cons of programmatically removing a trigger is something that should happen separate to this PR. IMO, our job here is to integrate workflow and maintain the same feature set. It shouldn't be about proposing fundamental plugin design changes.
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.
On Jul 27, 2015 12:48 AM, "Tom Fennelly" notifications@github.com wrote:
While that is true.. I don't know of a good way to implement this
functionality and it may make sense to do the PR to remove such a feature
first unless we can actually do this change..
One way might be to optionally depend on work flow plug in. I have seen
others do this but I don't know how they did it.
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.
@jacob-keller Right, I didn't see a way of doing it for workflow. So, the easiest thing (for now) might be to mark it as a missing feature for Workflow jobs and not have that point block this PR?
Then, separately have a discussion on the merits/demerits etc of auto-removing triggers. This will either result in a task to remove the feature completely (for all build types), or a task to find a way of making it work for workflow.
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 think that is fine myself. I would rather not block trigger support just
because we are missing a relatively minor feature.
On Jul 27, 2015 3:42 AM, "Tom Fennelly" notifications@github.com wrote:
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.
Agreed with @tfennelly’s summary.