-
Notifications
You must be signed in to change notification settings - Fork 34
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
Implement workflow support #10
base: master
Are you sure you want to change the base?
Conversation
…ist of developers.
Fix configuration persistence
Change away from DataBoundConstructor (the empty default is still needed) to DataBoundSetters to allow both multiple pools and plain ports in a single wrap. The legacy single pool variant is still supported.
The serialize inner class cases are just suppressed for now.
@oldelvet @pepov Would it be possible for you to review this one for merge into the main master? I see that one of the forks appears to have an integrated version and users claim a version running on their master in https://issues.jenkins-ci.org/browse/JENKINS-31449 |
Sorry I'm not active anymore in this project and don't have the time right now to get up to speed again. |
@@ -254,5 +256,11 @@ public Integer call() throws IOException { | |||
} | |||
|
|||
private static final long serialVersionUID = 1L; | |||
|
|||
@Override | |||
public void checkRoles(RoleChecker roleChecker) throws SecurityException { |
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.
Do not do this! I think you meant to just extend MasterToSlaveCallable
.
} | ||
|
||
@DataBoundSetter | ||
public void setPools(String[] pools) { |
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.
Why two attributes? Delete setPool
.
public void setPlainports(String[] plainports) { | ||
if (plainports != null) { | ||
for (String port : plainports) { | ||
this.ports.add(new DefaultPortType(port)); |
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.
Seems like you really wanted to have a List<PortType>
attribute, with each type being a Describable
with its own Descriptor
, config.jelly
, and @Symbol
.
public class PortAllocationWorkflowTest { | ||
|
||
@Rule | ||
public JenkinsRule j = new JenkinsRule(); |
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.
To meaningfully test anything relating to serialization, you will need to use RestartableJenkinsRule
.
I think you should be looking at the xvnc
plugin for examples of analogous work.
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p"); | ||
p.setDefinition(new CpsFlowDefinition( | ||
"node('slave') {\n" | ||
+ " wrap([$class: 'PortAllocator', pools: ['WEBLOGIC']]) {\n" |
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.
If you have a @Symbol
you should not need to explicitly use the wrap
step.
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.
@jglick can you give me an example plugin that does this? trying to resurrect this PR
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.
never mind, think I found what I need: https://jenkins.io/doc/developer/plugin-development/pipeline-integration/
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.
@jglick I started resurrecting this pr: https://github.com/er1c/port-allocator-plugin/tree/workflow-support but I got stuck on this @Symbol
thing.
I get an error like:
java.lang.NoSuchMethodError: No such DSL method portallocator found among [archive, bat, build, catchError, checkout, dir, echo, error, fff, fileExists, injectSubtypesAsContext, input, load, mail, node, parallel, persistenceProblem, pwd, readFile, retry, sh, sleep, stage, step, timeout, tool, unarchive, waitUntil, withEnv, wrap, writeFile, ws]
at org.jenkinsci.plugins.workflow.cps.DSL.invokeMethod(DSL.java:107)
at org.jenkinsci.plugins.workflow.cps.CpsScript.invokeMethod(CpsScript.java:98)
at org.codehaus.groovy.runtime.callsite.PogoMetaClassSite.call(PogoMetaClassSite.java:48)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
Is this something you can take a quick look at?
@bpedersen2 are you planning to address jglick's comments and close this anytime soon? |
Probably not anytime soon.... |
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.
No current plans to review again. Sounds like it needs someone to pick it up and do a bit of polish.
based on the work by @m00re implement workflow support for with a dedicated step.