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

[WIP]Make compatible with pipelines #2

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

Conversation

gjorando
Copy link

@gjorando gjorando commented Sep 12, 2017

The purpose of this PR is to get the plug-in to work with the (not so) new pipeline/workflow system of Jenkins. Since the repo hasn't been updated for a looot of time, there are also some dependencies updates.

I'd appreciate some feedback, as I'm not at all a expert Jenkins API programer nor a specialist of Jenkins it self. For what I tested on my own instance, my commits don't seem to break the plug-in. :)

TODO:

  • Bring compatibility with pipelines
  • Update dependencies
  • Update jelly files as variables need to be escaped by default
  • Update plugin maintainers list in pom.xml
  • Define a pipeline symbol for the build step
  • Update the README file
  • Delete the old perform method
  • In CcccParser, extend MasterToSlaveFileCallable instead of overriding checkRoles.
  • Add a project view.
  • Add unit tests to be sure that everything works as expected.
  • If everything is good, bump version and prepare the release

@gjorando gjorando changed the title Make this plug-in compatible with Jenkins' pipeline system [WIP]Make compatible with pipelines Nov 29, 2017
@@ -112,4 +118,40 @@ public String getMetricFilePath() {
return metricFilePath;
}

@Override
public void perform(@Nonnull Run<?, ?> run, @Nonnull FilePath filePath, @Nonnull Launcher launcher, @Nonnull TaskListener taskListener) {
Copy link
Member

Choose a reason for hiding this comment

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

This should replace the old perform method iirc, not added as an extra.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I'll change that!

@@ -223,4 +224,8 @@ public void setResultFilePath(FilePath resultFilePath) {
this.resultFilePath = resultFilePath;
}

@Override
public void checkRoles(RoleChecker roleChecker) throws SecurityException {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Oh, this is part of the legacy code I didn't change. But I can modify that too.

@gjorando
Copy link
Author

gjorando commented Dec 1, 2017

@rsandell Thank you for your comments, I did the requested modifications and tested them.

@rsandell
Copy link
Member

rsandell commented Dec 1, 2017

👍 LGTM

@rsandell
Copy link
Member

rsandell commented Dec 1, 2017

Though you should add a few tests that use a pipeline job to verify it works as intended :)

@gjorando
Copy link
Author

gjorando commented Dec 1, 2017

Ok! I'll add some tests, and update the README file.

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