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-49586] - Stop Serializing JDependParser to the disk (JEP-200 in 2.102+) #2

Merged
merged 3 commits into from Feb 22, 2018

Conversation

Projects
None yet
3 participants
@oleg-nenashev
Copy link
Member

oleg-nenashev commented Feb 16, 2018

https://issues.jenkins-ci.org/browse/JENKINS-49586

  • Facelift the plugin
  • Add Jenkinsfile
  • Fix compatibility with Jenkins 2.102+

@reviewbybees @jglick

@oleg-nenashev oleg-nenashev requested a review from jglick Feb 16, 2018

@jglick

jglick approved these changes Feb 16, 2018

@@ -0,0 +1,2 @@
// Build the plugin using https://github.com/jenkins-infra/pipeline-library
buildPlugin(jenkinsVersions: [null, '2.104'])

This comment has been minimized.

@jglick
/**
* @deprecated Use {@link #JDependBuildAction(JDependParser)}
*/
@Deprecated

This comment has been minimized.

@jglick

jglick Feb 16, 2018

Member

Probably pointless if you are deleting the build field anyway. Who would be calling this method from outside anyway?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 16, 2018

Author Member

🤷‍♂️ Prefer to keep binary compatibility when it does not cost too much

{
public final AbstractBuild<?, ?> build;
private final JDependParser jDependParser;

This comment has been minimized.

@jglick

jglick Feb 16, 2018

Member

IIUC this is the actual fix—the plugin was for some reason saving an instance field for an object used only in the constructor? Sigh.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 16, 2018

Author Member

Yeah, no idea why the maintainers wanted it. Maybe there were some plans to do fancy reporting instead of storing HTMLs (e.g. trends)

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Feb 22, 2018

@ndeloof could you please review/release that?

@ndeloof
Copy link
Member

ndeloof left a comment

@oleg-nenashev you probably get confused because last commits are mines, at a time the "everyone" team was used to commit and release arbitrary plugin (I did this for support reasons). I don't know much about this plugin's design

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Feb 22, 2018

@ndeloof OK, I have thought that formally you are a maintainer. Will request in the ML

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Feb 22, 2018

@ndeloof I discussed it with @daniel-beck, and we agreed that your approval is enough in this case. Could you please approve me taking ownership of this plugin for a while to spin the release? After some time I will mark it for adoption, the plan is similar to CppNCSS (https://groups.google.com/forum/#!msg/jenkinsci-dev/YU14gcHxJDg/CT5LpS1jBwAJ;context-place=forum/jenkinsci-dev)

@ndeloof

This comment has been minimized.

Copy link
Member

ndeloof commented Feb 22, 2018

@oleg-nenashev go for it

@oleg-nenashev oleg-nenashev merged commit afcf6bf into jenkinsci:master Feb 22, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit 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.