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

Reduce applicability requirements #32

Merged
merged 2 commits into from
Dec 1, 2015
Merged

Reduce applicability requirements #32

merged 2 commits into from
Dec 1, 2015

Conversation

dsavenko
Copy link
Contributor

Hi,

We have a project type, which does not extend AbstractProject, but directly extends Job and implements Queue.Task (we have certain reasons to not extend AbstractProject). We would like to have the Throttle plugin working with it, but it's tied to AbstractProject.

This pull request unties it from AbstractProject class and makes so, that the Throttle plugin works with any class which extends Job and implements Queue.Task.

This is just a technical patch, no new features added.

// Moving category to categories, to support, well, multiple categories per job.
@Deprecated transient String category;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid such formatting changes to simplify merges

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will get rid of them. Actually, I removed them manually before commit, but IDE put them again :)

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@jglick
Copy link
Member

jglick commented Nov 14, 2015

You probably want to implement ParameterizedJobMixIn.ParameterizedJob and have TCB look for that instead.

@dsavenko
Copy link
Contributor Author

Good point, I haven't thought about it. Will provide the update soon.

@dsavenko
Copy link
Contributor Author

@jglick, hmm... On second thought, ParameterizedJobMixIn is there since 1.556, while TCB is built against 1.424. Also, JobProperty's generic parameter must extend Job, so I'm bound to extend the ThrottleJobProperty class from JobProperty < Job > (can't use ParameterizedJobMixIn.ParameterizedJob, as it doesn't extend Job).

@oleg-nenashev
Copy link
Member

Personally I don't like the SometihgMixIn approach much, because it mostly workarounds Java's inheritance mechanism. It does not mean I can propose a better solution...

ParameterizedJobMixIn.ParameterizedJob is also not guaranteed to be used by all Job implementations, so I'd prefer the current approach.

In any case, the core dependency update may be reasonable. Versions 1.535...1.609.2 do not behave reliably, so probably bumping to 1.609.2 is the best option. The plugin needs some redesign, so please do not consider the version dependency as a blocker.

@dsavenko
Copy link
Contributor Author

So, what should I do next?

@abayer
Copy link
Member

abayer commented Dec 1, 2015

Dependency update is perfectly fine, yeah.

@abayer
Copy link
Member

abayer commented Dec 1, 2015

fwiw, tested this and it works fine throttling groups of Workflow jobs - not being used within the Workflow script, though. That's more work, which I'll get diving into once this is in.

@oleg-nenashev
Copy link
Member

👍 for the current implementation

@abayer
Copy link
Member

abayer commented Dec 1, 2015

Rightie-o, then I'll merge it.

abayer added a commit that referenced this pull request Dec 1, 2015
Reduce applicability requirements
@abayer abayer merged commit 7a6b581 into jenkinsci:master Dec 1, 2015
@dsavenko
Copy link
Contributor Author

dsavenko commented Dec 2, 2015

Thanks for merging!

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

Successfully merging this pull request may close these issues.

5 participants