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

Enable release plugin for ivy plugin with optional dependency #14

Merged
merged 4 commits into from Nov 18, 2016

Conversation

Projects
None yet
3 participants
@fbuehlmann
Copy link
Contributor

fbuehlmann commented Dec 17, 2015

Add release functionality to 3rd party plugins without code dependency
There are two plugins that could also benefit from release funtionality:

  • Ivy Plugin

I integrated the functionality to these plguins without getting dependency to the plugin code.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Dec 28, 2015

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

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Sep 28, 2016

I'm not sure how this change is supposed to work. Needs clarification

@fbuehlmann

This comment has been minimized.

Copy link
Contributor Author

fbuehlmann commented Sep 28, 2016

There was one commit missing in my request. Therefore I reimplemented it right now.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Sep 28, 2016

Well, it messes up the code even more :(
Could you get rid of unrelated formatting changes?

@fbuehlmann fbuehlmann force-pushed the buhlmann:master branch 2 times, most recently from 6399aa2 to db80436 Sep 28, 2016

@fbuehlmann

This comment has been minimized.

Copy link
Contributor Author

fbuehlmann commented Sep 29, 2016

Now only the changed code is in the commits. Sorry for that.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Sep 29, 2016

Great, now it is much more reviewable :)

Generally, I would vouch for adding optional dependencies to these plugins (with referencing compatible versions) and maybe writing some tests. It's not really required, because ideally this plugin should provide an extension point, which would allow other plugins to declare the compatibility. So I consider it only as a temporary fix (CC @jenkinsci/code-reviewers).

Maybe it's also better to use full class names since generic class names "GroupJobProject" may potentially overlap with other OSS or internal plugins.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Oct 12, 2016

@fbuehlmann gentle ping. I could merge it as-is, but I would to get clarification regarding usage of not-fully-qualified names

@fbuehlmann

This comment has been minimized.

Copy link
Contributor Author

fbuehlmann commented Oct 25, 2016

I would refactor to optional dependencies. This sounds the best to me.

@fbuehlmann fbuehlmann force-pushed the buhlmann:master branch from 44e96f9 to 2206d60 Oct 31, 2016

@fbuehlmann fbuehlmann force-pushed the buhlmann:master branch from 67459e8 to cf458ae Oct 31, 2016

@fbuehlmann fbuehlmann changed the title Enable release plugin for ivy and group job plugins without code Enable release plugin for ivy plugin with optional dependency Oct 31, 2016

@fbuehlmann fbuehlmann force-pushed the buhlmann:master branch from 1985ca8 to ba9a64d Oct 31, 2016

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Oct 31, 2016

LGTM

@oleg-nenashev oleg-nenashev merged commit 69187e5 into jenkinsci:master Nov 18, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.