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

[JENKINS-35571] Make compatible with Pipeline #30

Closed
wants to merge 22 commits into from

Conversation

4 participants
@vgaidarji
Copy link
Contributor

commented Jul 25, 2017

This PR brings plugin compatibility with Jenkins Pipeline.
Closes JENKINS-35571.

Background:
The work in this PR is based on MarkusDNC/plot-plugin repository. That repo contains original plot plugin adjustment for Jenkins Pipeline (works only with Pipeline). In order to not to break compatibility I diff'ed every file from modified plugin repo with the original repo. This was a painful process, but since I'm not quite into plot plugin, I decided to go this way since the work was done and I had to combine it with the existing codebase.
As result, not all changes were applicable on top of existing functionality and I put the difference into pipeline package. There might be a better way though.
image

I tried to combine
MatrixPlotPublisher & Plot & MatrixPlotAction classes with
pipeline/PlotPublisher & pipeline/PlotBuilder & pipeline/PlotAction but faced incompatibilities and decided to leave those separated.

Additionally, I did some code clean up and fixed PMD warnings.

Tests

  • Unit tests are passing
  • Plot plugin works from Pipeline job
    plot-pipeline
  • Plot plugin works from Matrix job
    plot-matrix

TODO:

  • Update plugin docs (add plugin sample usage from Pipeline)
  • Update Compatibility document

vgaidarji added some commits Jul 6, 2017

Bring in plot pipeline project classes
Based on initiate from separate repo https://github.com/MarkusDNC/plot-plugin.
This repo isn't supported anymore, but adds compatibility with Jenkins Pipeline.
Rework modified classes for pipeline
Adjusted existing classes with changes from modified plugin version.
Classes which are too different from existing ones were placed into 'pipeline' package.
Existing classes might be adjusted to be compatible with pipeline, but not yet clear how to achieve that.
@ericbn

This comment has been minimized.

Copy link
Member

commented Jul 25, 2017

This plugin is up for adoption. Want to help improve this plugin? Click here to learn more!

@vgaidarji vgaidarji self-assigned this Aug 31, 2017

@rtyler

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

@vgaidarji i'm not sure why this isn't rebuilding on ci.jenkins.io now that #31 has been merged. If you close and re-open the pull request, that should trigger a new GitHub webhook event 🔥

@vgaidarji

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

@rtyler I'm not even sure it should trigger the build as nothing actually changed in this PR.
but yeah, I'll make sure build passes before I merge this one. thanks

@vgaidarji vgaidarji closed this Aug 31, 2017

@vgaidarji vgaidarji reopened this Aug 31, 2017

@rtyler

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

YAY, success! 🎆

vgaidarji added some commits Aug 31, 2017

@svanoort

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

I (@svanoort) can't mark it due to lack of repo permissions but I am reviewing (may take a few days to get results)

@vgaidarji

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

@svanoort I would like to do more clean up in the repo. Would be nice if you start the review in few hours. Basically, no logic change will be done, and clean up will be done in separate commits. We should be fine reviewing commit by commit.

@vgaidarji

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

Resetting this branch to d2b11f3 and moving clean up activities to separate PR to ease the review and separate concerns.

@vgaidarji

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

This PR originated from the fork, which was removed.
@svanoort FYI: further steps will be in GH-32.

@vgaidarji vgaidarji closed this Aug 31, 2017

@vgaidarji vgaidarji referenced this pull request Aug 31, 2017

Merged

[JENKINS-35571] Make compatible with Pipeline #32

5 of 5 tasks complete
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.