-
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
Added workflow/pipeline support for pooled ports. #8
base: master
Are you sure you want to change the base?
Conversation
@@ -29,8 +29,15 @@ | |||
<id>pepov</id> | |||
<name>Peter Wilcsinszky</name> | |||
</developer> | |||
<developer> | |||
<name>Jens Mittag</name> |
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 you really plan to be developer for this project? If you are contributing single change and would handle and support it later, don't yourself as developer.
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 have no plans to maintain the plugin. I only added myself in the list because I assumed it resembles all past contributors. I have no issue with removing my name from here.
…ist of developers.
What do we need to have this merged? I would really need this to migrate a job to pipeline. |
@kariem, I can't speak to their release/merge process, but you can always cherry-pick this and build the branch yourself by cloning this repository, cherry-picking these commits and running "mvn install" in the root of the project. This will generate an hpi file that you can install using the advanced plugin manager within Jenkins. |
@@ -36,14 +40,21 @@ private PortAllocator(PortType[] ports){ | |||
this.ports = ports; | |||
} | |||
|
|||
@DataBoundConstructor | |||
public PortAllocator(String pool) { |
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 might be backward incompatible. What will happen to instances already serialized by the previous version of this class?
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.
Can you elaborate a little bit on this potential issue? Do you mean serialized in the context of a pipeline job (which was not supported by this plugin prior to this PR)? From what I understand, non-pipeline builds can not be serialized and paused/halted.
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, I mean in the config.xml
of the job.
To check if it's backward compatible: 1) install the previous version of the plugin and configure this build wrapper in a job, then 2) upgrade the plugin to this snapshot and check if the job configuration is still correct and nothing is missing
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 can confirm this is working. We have a total of 60 build jobs in our company, and we still have approx. 20 jobs that haven't been migrated to a Pipeline job structure yet. Both types of jobs work perfectly together, no issues w.r.t. co-existence or build failures after plugin upgrade.
Looks good in general, but a backward compatibility check test would be required IMO (using |
I am currently preparing a followup to allow more than one pool and also plain ports. Pull request comming soon. |
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.
Wouldn't you merge this one first? You can still make enhancements afterwords. |
I based my changes on this patch, so yes this one can get merged first. |
Implement multiple pools and ports for workflow
After Dec 03, 2016 not seeing any comments. Any plans for pipeline support? |
I would welcome a release for usage with pipelines as well. |
when Can this functionality be merged? |
This is a set of changes that make the port-allocator plugin compatible with the workflow API. I don't know whether evyrthing is 100% correct (this is my first work on a Jenkins plugin), but on my instance everything works as expected.
Usage:
So far, I only added support for Pooled Ports (from within the pipeline). The changes also make sure that you can mix between classic job descriptions and pipeline projects.
What is not yet working: surviving Jenkins restarts. For us/me, this is not a requirement, but I am willing to add support for this too if someone provides me the necessary pointers (I already tried, but my knowledge of Jenkins plugin interna is very limited).
I am looking forward to your comments/feedback. Constructive suggestions welcome.