[JENKINS-26010] Workflow compatibility #240
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
| import hudson.Functions; | ||
| import hudson.RelativePath; | ||
| import hudson.model.AbstractProject; | ||
| import hudson.model.Job; | ||
| import hudson.model.Action; | ||
| import hudson.model.Describable; | ||
| import hudson.model.Failure; | ||
|
|
@@ -904,7 +905,7 @@ private void rename(String newName) { | |
| * | ||
| * @return the list of jobs configured with this server. | ||
| */ | ||
| public List<Job> getConfiguredJobs() { | ||
| return PluginImpl.getConfiguredJobs_(name); | ||
| } | ||
|
|
||
|
|
@@ -914,14 +915,23 @@ public List<AbstractProject> getConfiguredJobs() { | |
| * @param oldName the old name of the Gerrit server | ||
| */ | ||
| private void changeSelectedServerInJobs(String oldName) { | ||
| for (Job job : PluginImpl.getConfiguredJobs_(oldName)) { | ||
|
|
||
| if (!(job instanceof AbstractProject)) { | ||
| logger.info("Unable to modify Gerrit Trigger configurations for job [" + job.getName() | ||
| + "] after Gerrit server has been renamed from [" + oldName + "] to [" + name + "]." | ||
| + " This feature is only supported for AbstractProject types e.g. Freestyle Jobs."); | ||
| return; | ||
|
There was a problem hiding this comment. What would happen if it's an instance of There was a problem hiding this comment. 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. The There was a problem hiding this comment. @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 Trigger There was a problem hiding this comment. 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. 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. 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 One way might be to optionally depend on work flow plug in. I have seen There was a problem hiding this comment. @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. I think that is fine myself. I would rather not block trigger support just
There was a problem hiding this comment. Agreed with @tfennelly’s summary. |
||
| } | ||
| AbstractProject project = (AbstractProject)job; | ||
|
|
||
| GerritTrigger trigger = (GerritTrigger)project.getTrigger(GerritTrigger.class); | ||
| if (trigger != null) { | ||
| try { | ||
| trigger.setServerName(name); | ||
| trigger.start(job, false); | ||
| project.addTrigger(trigger); | ||
| project.save(); | ||
| } catch (IOException e) { | ||
| logger.error("Error saving Gerrit Trigger configurations for job [" + job.getName() | ||
| + "] after Gerrit server has been renamed from [" + oldName + "] to [" + name + "]"); | ||
|
|
@@ -934,11 +944,19 @@ private void changeSelectedServerInJobs(String oldName) { | |
| * Remove "Gerrit event" as a trigger in all jobs selecting this server. | ||
| */ | ||
| private void removeGerritTriggerInJobs() { | ||
| for (Job job : getConfiguredJobs()) { | ||
|
|
||
| if (!(job instanceof AbstractProject)) { | ||
| logger.info("Unable to remove Gerrit Trigger ffrom job [" + job.getName() + "]. " | ||
|
|
||
| + " This feature is only supported for AbstractProject types e.g. Freestyle Jobs."); | ||
| return; | ||
| } | ||
| AbstractProject project = (AbstractProject)job; | ||
|
|
||
| GerritTrigger trigger = (GerritTrigger)project.getTrigger(GerritTrigger.class); | ||
| trigger.stop(); | ||
| try { | ||
| project.removeTrigger(trigger.getDescriptor()); | ||
| } catch (IOException e) { | ||
| logger.error("Error removing Gerrit trigger from job [" + job.getName() | ||
| + "]. Please check job config"); | ||
|
|
||
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.
this should be
warninginstead ofinfo