Skip to content
This repository has been archived by the owner on May 28, 2024. It is now read-only.

[JEP-200] POM updates #16

Closed
wants to merge 1 commit into from
Closed

[JEP-200] POM updates #16

wants to merge 1 commit into from

Conversation

jglick
Copy link
Member

@jglick jglick commented Jan 12, 2018

Routine POM cleanups, permitting tests to be run against newer Jenkins versions than the declared baseline.

Attempting to test against jenkinsci/jenkins#3120, since code inspection suggests that the RemoteScanResult.scan field will fail to be deserialized from agents in Jenkins 2.102+. Unfortunately you appear to have no functional tests whatsoever which would build on agents; when I tried

diff --git a/src/test/java/org/sonatype/nexus/ci/iq/IqPolicyEvaluatorIntegrationTest.groovy b/src/test/java/org/sonatype/nexus/ci/iq/IqPolicyEvaluatorIntegrationTest.groovy
index c3be5a2..767d944 100644
--- a/src/test/java/org/sonatype/nexus/ci/iq/IqPolicyEvaluatorIntegrationTest.groovy
+++ b/src/test/java/org/sonatype/nexus/ci/iq/IqPolicyEvaluatorIntegrationTest.groovy
@@ -108,6 +108,7 @@ class IqPolicyEvaluatorIntegrationTest
   def 'Freestyle build (happy path)'() {
     given: 'a jenkins project'
       FreeStyleProject project = jenkins.createFreeStyleProject()
+      project.assignedNode = jenkins.createSlave()
       project.buildersList.add(new IqPolicyEvaluatorBuildStep('stage', 'app', [], false, 'cred-id'))
       configureJenkins()

it did not work, I suppose because you are using Java mocks rather than an actual mock (HTTP?) service, and mocks do not transfer across JVM boundaries.

I also have no idea how to test this plugin manually, and no inclination to try. The regression, if real, could most likely be corrected by creating a src/main/resources/META-INF/hudson.remoting.ClassFilter containing

com.sonatype.insight.scan.model.Scan
com.sonatype.insight.scan.model.Application
# …whatever the transitive serial closure of Scan might be

or by simply making the Remoting channel transfer only simpler datatypes, such as String, rather than relying on serialization of external libraries.

@reviewbybees

@ghost
Copy link

ghost commented Jan 12, 2018

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

So good so far. POM definitely needs more cleanup to make the build green eventually

@@ -483,7 +413,6 @@
<id>jenkins-1.642.3</id>
<properties>
<jenkins.version>1.642.3</jenkins.version>
<jenkins-test-harness.version>1.642.3</jenkins-test-harness.version>
Copy link
Member

Choose a reason for hiding this comment

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

Not required though should not harm as well

<findbugs-maven-plugin.version>3.0.2</findbugs-maven-plugin.version>
<jvnet-localizer-plugin.version>1.23</jvnet-localizer-plugin.version>
<concurrency>1</concurrency>
<forkCount>1</forkCount>
<java.level>7</java.level>
<slf4j.version>1.7.7</slf4j.version>
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be removed for Plugin POM 3.2 IIRC

@whyjustin
Copy link
Member

whyjustin commented Jan 13, 2018

Thanks for the alert on JENKINS-47736 and JEP 200. Will look into the feasibility of using simple data types or instead utilize src/main/resources/META-INF/hudson.remoting.ClassFilter. This is a fairly large change (2.22 -> 3.2) we'll do a quick review before merging.

@oleg-nenashev
Copy link
Member

@whyjustin
Copy link
Member

Took @jglick changes, added whitelist and supporting tests for remote slaves in #17. Thanks again. Closing in favor of #17.

@whyjustin whyjustin closed this Jan 16, 2018
@jglick jglick deleted the pom branch January 16, 2018 22:58
bigspotteddog pushed a commit that referenced this pull request Jul 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants