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-49574] - Make the plugin compatible with PCT and fix the compatibility with Jenkins 2.102+ #45

Merged
merged 11 commits into from Feb 28, 2018

Conversation

@oleg-nenashev
Copy link
Member

commented Feb 16, 2018

  • Plugin facelift, 1.625.3 is the new baseline
  • Jenkinsfile
  • Fix some FindBugs and Javadoc issues
  • Fix JENKINS-49574
  • Create test for JENKINS-49574 so that PCT catches it

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

@reviewbybees @jglick

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

cf = ClassFilter.DEFAULT;
}

//TODO: It checks abstract classes, but not implementation

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 16, 2018

Author Member

The code below is specific to Remoting serialization ONLY, because abstract classes are checked. It's not important for XStream. Ideally this code should be moved to JTH somehow

This comment has been minimized.

Copy link
@jglick
@@ -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.

Copy link
@jglick

@Test
@Issue("JENKINS-49574") // Fails on Jenkins 2.102+
public void checkClassesForDefaultRemotingBlacklist() {

This comment has been minimized.

Copy link
@jglick

jglick Feb 16, 2018

Member

Delete this stuff. If there is an actual reproducible regression, we need a functional test that demonstrates it. A CVS server can be easily packaged in a Docker fixture so there is no excuse not to.

This comment has been minimized.

Copy link
@jglick

jglick Feb 16, 2018

Member

In fact there is already IntegrationTest, though it is probably not running on CI, pending use of docker-fixtures.

This comment has been minimized.

Copy link
@jglick

jglick Feb 16, 2018

Member

Just need to complete

// TODO check changelog
cf = ClassFilter.DEFAULT;
}

//TODO: It checks abstract classes, but not implementation

This comment has been minimized.

Copy link
@jglick
@@ -0,0 +1,10 @@
# For JENKINS-49574
# Calendar class implementation has a custom deserialization logic, but it seems to be safe.
java.util.Calendar

This comment has been minimized.

Copy link
@jglick

jglick Feb 16, 2018

Member

Kill. IIUC all this is just for CVSChangeLog.changeDate, right? So switch that to a long and be done with it.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

@jglick

Delete this stuff. If there is an actual reproducible regression, we need a functional test that demonstrates it. A CVS server can be easily packaged in a Docker fixture so there is no excuse not to.

With all due respect, I do not want to spend time on writing functional tests for CVS in 2018. It is not only a docker rule, but also much overhead (configure sample project, remember what CVS was about for starters, etc.). This is also the reason why I do not want to modify CVSChangeLog.changeDate.

I would prefer to keep the PR as is and to leave refactorings/test automation to the plugin maintainer (@fredg02, do you maintain it now?)

@oleg-nenashev oleg-nenashev requested review from jglick and fredg02 Feb 19, 2018

@jglick
Copy link
Member

left a comment

I am not comfortable with merging this as is. I will try to write an amending PR.

@jglick jglick merged commit 6f2103c into jenkinsci:master Feb 28, 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
Projects
None yet
2 participants
You can’t perform that action at this time.