[JENKINS-55568] Add support of passing Jenkins Core version from pom.xml #72

fcojfernandez · 2019-02-27T17:34:53Z

See JENKINS-55568

Add support of passing Jenkins Core version from pom.xml:

In case the jenkins.version is specified, the war section in yml file is overridden and ignored
In case a dependency on jenkins-core or jenkins-war exist, the dependency overrides any other configuration for the core.

Besides, the documentation is updated and I've added a test.

oleg-nenashev

Thanks @fcojfernandez !

The code itself looks good, but I have some concerns about the behavior.

The code still requires the war section to be present in the config YAML, even if it is unused
It will be a breaking change for users who already use pom.xml as an input and use Jenkins core version defined in YAML. . For example, https://github.com/jenkinsci/ci.jenkins.io-runner

My proposal would be to slightly alter the logic so that...

Jenkins core version in packager-config.yml is a priority. A warning is printed if it overrides pom.xml
If war section is not specified, pom.xml or BOM are used
If Jenkins WAR is not specified anywhere, the build just fails

I would like to also note that you might want to use jenkins-war.version if it is specified. Jenkins Core and Jenkins WAR versions may be different in some cases (e.g. timestamped snapshots)

varyvol · 2019-03-01T13:57:50Z

custom-war-packager-lib/src/main/java/io/jenkins/tools/warpackager/lib/config/Config.java

@@ -181,6 +182,13 @@ public void overrideByPOM(@Nonnull File tmpDir, @Nonnull File pom, final boolean
        }

        MavenHelper helper = new MavenHelper(this);
+        String jenkinsVersion = model.getProperties().getProperty("jenkins.version");
+        if (StringUtils.isNotBlank(jenkinsVersion)) {
+            ComponentReference core = new ComponentReference();


Don't you need to specify other info here, such as the coordinates?

It's set in war = core.toWARDependencyInfo();

Yes, but I don't see any assignments done to such object, I guess it's created to content core references.

See here: https://github.com/jenkinsci/custom-war-packager/blob/master/custom-war-packager-lib/src/main/java/io/jenkins/tools/warpackager/lib/model/bom/ComponentReference.java#L47-L65

varyvol · 2019-03-01T13:58:27Z

custom-war-packager-lib/src/main/java/io/jenkins/tools/warpackager/lib/util/MavenHelper.java

@@ -143,6 +143,7 @@ public void downloadJAR(File buildDir, DependencyInfo dep, String version, File
                    DependencyInfo dep = new DependencyInfo();
                    dep.groupId = dependencyData[0].trim();
                    dep.artifactId = dependencyData[1].trim();
+                    dep.type = dependencyData[2].trim();


This could also be empty (as in the DependencyInfo changes)?

The dependency:list always informs the type, so here it cannot be null. In DependencyInfo I've checked the nullability just in case it could be used in other way.

fcojfernandez · 2019-03-04T08:51:24Z

@oleg-nenashev regarding your concerns:

The code still requires the war section to be present in the config YAML, even if it is unused

I've kept the current behaviour as with BOM. If the BOM or POM specify the jenkins version, it's overridden.

Jenkins core version in packager-config.yml is a priority. A warning is printed if it overrides pom.xml
If war section is not specified, pom.xml or BOM are used

This is the opposite behaviour comparing with plugins. IMHO, it makes sense to keep the coherence and the same behaviour. In fact, if the core is set in BOM, it already override the war section.

If Jenkins WAR is not specified anywhere, the build just fails

As I've mentioned before, I've kept the current behaviour. I can check what will happen (I think it's going to fail) and if not, I think this part makes sense.

Regarding your last comment, I'm not sure to be following you:

I would like to also note that you might want to use jenkins-war.version if it is specified. Jenkins Core and Jenkins WAR versions may be different in some cases (e.g. timestamped snapshots)

Are you proposing:

Read jenkins-war.version
If not exists, read jenkins.version

What about the dependencies? IMO, they should prevail before the properties.

fcojfernandez · 2019-03-04T10:06:05Z

@oleg-nenashev

If Jenkins WAR is not specified anywhere, the build just fails

It's currently failing, but the message does not give detailed information. My last commit is to check and throw an exception with a better information message

fcojfernandez · 2019-03-04T10:32:29Z

I would like to also note that you might want to use jenkins-war.version if it is specified. Jenkins Core and Jenkins WAR versions may be different in some cases (e.g. timestamped snapshots)

With my last commit I've added the support for reading the property since I agree with @oleg-nenashev that it makes sense:

Read jenkins-war.version
If not exist, read jenkins.version

The dependency section in pom prevails.

oleg-nenashev · 2019-03-04T10:39:01Z

@fcojfernandez

The dependency section in pom prevails.

The problem is that it will be a breaking change for existing users (core from packager-config.yml would be used before the change, pom.xml would be used afterwards). And there is no opt-out offered.

fcojfernandez · 2019-03-04T11:16:05Z

@oleg-nenashev I mean that dependency section prevails over the properties.

Regarding the pom prevailing over the yml, I see your point but it's the same behaviour than BOM has got: BOM overrides the war section, so for me, the natural behaviour for POM would be to mimic BOM. It seems weird to me if they behave differently.
@varyvol any thought? A third opinion would be useful here

oleg-nenashev · 2019-03-04T11:19:33Z

Regarding the pom prevailing over the yml, I see your point but it's the same behaviour than BOM has got: BOM overrides the war section, so for me, the natural behaviour for POM would be to mimic BOM. It seems weird to me if they behave differently.

I agree. I would be interested to work on a new major release with better YAML format (e.g. multi-source input and templating from JIRA), but it is not in the scope for now. I would just like to retain some level of compatibility if possible for now until the rework happens.

Maybe we could just have a bomIncludeWar flag or whatever?

fcojfernandez · 2019-03-04T11:28:00Z

You mean a global bomIncludeWar=true to override the war section? So if it's false or it's not specified then the war section is the source for configuring the war; and if it is not true and the war section is missing, then the build fails. This approach would work for me. Is it fine for you?

oleg-nenashev · 2019-03-04T11:40:03Z

This approach would work for me. Is it fine for you?

yup

custom-war-packager-lib/src/main/java/io/jenkins/tools/warpackager/lib/util/MavenHelper.java

varyvol · 2019-03-04T14:55:24Z

custom-war-packager-lib/src/main/java/io/jenkins/tools/warpackager/lib/config/Config.java

+            war = null;
+            String jenkinsVersion = model.getProperties().getProperty("jenkins-war.version");
+            if (StringUtils.isBlank(jenkinsVersion)) {
+                jenkinsVersion = model.getProperties().getProperty("jenkins.version");


I'm not sure it affects, but note that ComponentReference always uses the 'jenkins-war' artifact.

It's the artifact used to explode the war. See here:

custom-war-packager/custom-war-packager-lib/src/main/java/io/jenkins/tools/warpackager/lib/config/Config.java

Line 221 in 251cc15

war = spec.getCore().toWARDependencyInfo();

while the specification is

custom-war-packager/custom-war-packager-maven-plugin/src/it/bom/bom.yml

Lines 8 to 10 in 251cc15

spec:

core:

version: 2.118

varyvol · 2019-03-05T11:02:44Z

It seems tests are failing?

fcojfernandez · 2019-03-05T11:07:47Z

Something has interrupted the execution. Seen in the logs of a non-related (to this PR) test:

INFO: Generating Dockerfile

Sending interrupt signal to process

/home/jenkins/workspace/Tools_custom-war-packager_PR-72/jenkinsfile-runner-tests/.jenkinsfile-runner-test-framework/src/cwp/custom-war-packager.inc: line 82: 20627 Terminated              docker build -t "$image_tag" -f "$1/out/tmp/output/Dockerfile" "$1/out/tmp/output/"

Last time I saw this error, closing and re-opening didn't work, so I'll make a fake commit

fcojfernandez and others added 3 commits February 27, 2019 18:27

[JENKINS-55568] Jenkins Core version from pom.xml

ce66be7

[JENKINS-55568] Fix error in commit

77a6a18

Merge branch 'master' into JENKINS-55568

75363ac

fcojfernandez requested review from varyvol and oleg-nenashev February 27, 2019 17:36

oleg-nenashev requested changes Feb 28, 2019

View reviewed changes

varyvol reviewed Mar 1, 2019

View reviewed changes

[JENKINS-55568] Better error message

22d9f41

[JENKINS-55568] Add support for jenkins-war.version property

840b139

fcojfernandez closed this Mar 4, 2019

fcojfernandez reopened this Mar 4, 2019

[JENKINS-55568] Add bomIncludeWar flag

5e75cdb

varyvol reviewed Mar 4, 2019

View reviewed changes

fcojfernandez added 2 commits March 4, 2019 16:10

[JENKINS-55568] Wrong commit

9a5a28e

[JENKINS-55568] FIx findbugs

9c02642

fcojfernandez requested review from oleg-nenashev and varyvol March 5, 2019 10:04

fcojfernandez added 2 commits March 5, 2019 12:11

[JENKINS-55568] Fake commit to relaunch CI

e33bd24

[JENKINS-55568] Re-adjust the timeout value in Jenkinsfile

13fc0a7

fcojfernandez closed this Mar 6, 2019

fcojfernandez reopened this Mar 6, 2019

[JENKINS-55568] Fake commit to relaunch CI (again)

f5e495c

varyvol approved these changes Mar 6, 2019

View reviewed changes

oleg-nenashev approved these changes Mar 6, 2019

View reviewed changes

oleg-nenashev merged commit d82ca51 into jenkinsci:master Mar 6, 2019

fcojfernandez deleted the JENKINS-55568 branch March 6, 2019 15:58

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-55568] Add support of passing Jenkins Core version from pom.xml #72

[JENKINS-55568] Add support of passing Jenkins Core version from pom.xml #72

fcojfernandez commented Feb 27, 2019

oleg-nenashev left a comment

varyvol Mar 1, 2019

fcojfernandez Mar 4, 2019

varyvol Mar 4, 2019

fcojfernandez Mar 4, 2019

varyvol Mar 1, 2019

fcojfernandez Mar 4, 2019 •

edited

Loading

fcojfernandez commented Mar 4, 2019

fcojfernandez commented Mar 4, 2019 •

edited

Loading

fcojfernandez commented Mar 4, 2019

oleg-nenashev commented Mar 4, 2019

fcojfernandez commented Mar 4, 2019

oleg-nenashev commented Mar 4, 2019

fcojfernandez commented Mar 4, 2019

oleg-nenashev commented Mar 4, 2019

varyvol Mar 4, 2019

fcojfernandez Mar 4, 2019 •

edited

Loading

varyvol commented Mar 5, 2019

fcojfernandez commented Mar 5, 2019

[JENKINS-55568] Add support of passing Jenkins Core version from pom.xml #72

[JENKINS-55568] Add support of passing Jenkins Core version from pom.xml #72

Conversation

fcojfernandez commented Feb 27, 2019

oleg-nenashev left a comment

Choose a reason for hiding this comment

varyvol Mar 1, 2019

Choose a reason for hiding this comment

fcojfernandez Mar 4, 2019

Choose a reason for hiding this comment

varyvol Mar 4, 2019

Choose a reason for hiding this comment

fcojfernandez Mar 4, 2019

Choose a reason for hiding this comment

varyvol Mar 1, 2019

Choose a reason for hiding this comment

fcojfernandez Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

fcojfernandez commented Mar 4, 2019

fcojfernandez commented Mar 4, 2019 • edited Loading

fcojfernandez commented Mar 4, 2019

oleg-nenashev commented Mar 4, 2019

fcojfernandez commented Mar 4, 2019

oleg-nenashev commented Mar 4, 2019

fcojfernandez commented Mar 4, 2019

oleg-nenashev commented Mar 4, 2019

varyvol Mar 4, 2019

Choose a reason for hiding this comment

fcojfernandez Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

varyvol commented Mar 5, 2019

fcojfernandez commented Mar 5, 2019

fcojfernandez Mar 4, 2019 •

edited

Loading

fcojfernandez commented Mar 4, 2019 •

edited

Loading

fcojfernandez Mar 4, 2019 •

edited

Loading