Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

m00re
Copy link

@m00re m00re commented Jul 24, 2016

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:

wrap([$class: 'PortAllocator', pool: 'POOLNAME']) {
}

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.

@@ -29,8 +29,15 @@
<id>pepov</id>
<name>Peter Wilcsinszky</name>
</developer>
<developer>
<name>Jens Mittag</name>
Copy link
Member

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.

Copy link
Author

@m00re m00re Jul 25, 2016

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.

@kariem
Copy link

kariem commented Sep 6, 2016

What do we need to have this merged? I would really need this to migrate a job to pipeline.

@TheRhino04
Copy link

@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) {
Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

@amuniz amuniz Oct 5, 2016

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

Copy link
Author

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.

@amuniz
Copy link
Member

amuniz commented Sep 17, 2016

Looks good in general, but a backward compatibility check test would be required IMO (using @LocalData).

@bpedersen2
Copy link

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.
@romixch
Copy link

romixch commented Dec 2, 2016

Wouldn't you merge this one first? You can still make enhancements afterwords.

@bpedersen2
Copy link

I based my changes on this patch, so yes this one can get merged first.

Implement multiple pools and ports for workflow
@nsidhaye
Copy link

After Dec 03, 2016 not seeing any comments. Any plans for pipeline support?

@scic
Copy link

scic commented Apr 25, 2018

I would welcome a release for usage with pipelines as well.

@bharath0080
Copy link

when Can this functionality be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.