Skip to content

Added workflow/pipeline support for pooled ports.#8

Open
m00re wants to merge 7 commits into
jenkinsci:masterfrom
m00re:master
Open

Added workflow/pipeline support for pooled ports.#8
m00re wants to merge 7 commits into
jenkinsci:masterfrom
m00re:master

Conversation

@m00re
Copy link
Copy Markdown

@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.

Comment thread pom.xml Outdated
<name>Peter Wilcsinszky</name>
</developer>
<developer>
<name>Jens Mittag</name>
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

@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.

}

@DataBoundConstructor
public PortAllocator(String pool) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

romixch commented Dec 2, 2016

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

@bpedersen2
Copy link
Copy Markdown

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
Copy Markdown

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

@scic
Copy link
Copy Markdown

scic commented Apr 25, 2018

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

@bharath0080
Copy link
Copy Markdown

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.