chore(Jeninsfile) Switch Maven commands to infra.runWithMaven #5843

dduportal · 2021-10-21T12:42:49Z

The goal is to benefits from infra's JDK and maven setups per agent kind.

It avoid infamous errors The JAVA_HOME environment variable is not defined correctly, in the ATH and benefits the work from jenkins-infra/pipeline-library#231 abd jenkins-infra/pipeline-library#232 (not downloading JDK on each agent)

Related to #5841 which "ath" step failed.

First manual test (used "replay" with the PR's pipeline as I'm not maintainer,but admin on ci.jenkins.io): https://ci.jenkins.io/job/Core/job/jenkins/job/PR-5843/11/

Proposed changelog entries

N.A.

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@jglick @timja

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

jglick

Accidental deletion of important git diff call.

Jenkinsfile

dduportal · 2021-10-21T16:18:09Z

@jglick many thanks for your review (I learned about the $WORKSPACE_TMP variable). I tried to take all in account, and you can find the "replay" build with the pipeline content : https://ci.jenkins.io/job/Core/job/jenkins/job/PR-5843/14/console

jglick

Looks like it should work.

Jenkinsfile

jglick

As a rule, I really dislike runMaven and think it should be deprecated. Prefer runWithMaven + sh 'mvn …' for transparency. Would that suffice to solve the issues mentioned in the description?

jglick · 2021-10-21T16:31:06Z

Jenkinsfile

+                            runTests ? '-Dmaven.test.failure.ignore' : '-DskipTests',
+                            "-V",
+                            "-B",
+                            "-ntp", // Requires Maven >= 3.6.1


Apparently already part of runMaven?

Correct: https://github.com/jenkins-infra/pipeline-library/blob/master/vars/infra.groovy#L87

dduportal · 2021-10-21T16:35:51Z

As a rule, I really dislike runMaven and think it should be deprecated. Prefer runWithMaven + sh 'mvn …' for transparency. Would that suffice to solve the issues mentioned in the description?

Make sense, it sounds like we can pass a closue block to runWithMaven which avoid the tedious string lists.
I'll try with this instead but tomorrow. Thanks @jglick

The goal is to benefit from the the shared library jenkins-infra/pipeline-library that manages agents and tools. Signed-off-by: Damien Duportal <[email protected]>

dduportal · 2021-10-22T08:41:18Z

@jglick @timja I've reworked the PR to use the infra.runWithMaven() command instead. WDYT?

"Replayed" Job to test the pipeline changes: https://ci.jenkins.io/job/Core/job/jenkins/job/PR-5843/18/console

timja · 2021-10-22T08:50:40Z

Jenkinsfile

@@ -10,6 +10,8 @@ def buildNumber = BUILD_NUMBER as int; if (buildNumber > 1) milestone(buildNumbe
 // TEST FLAG - to make it easier to turn on/off unit tests for speeding up access to later stuff.
 def runTests = true
 def failFast = false
+// Same memory sizing for both builds and ATH
+def javaOpts = ["JAVA_OPTS=-Xmx1536m -Xms512m","MAVEN_OPTS=-Xmx1536m -Xms512m"]


not directly related to this PR but is this why the tests are so darn slow. cc @basil

Locally my Jenkins core tests are soo much faster, our agents have far more resources than this =/

Mmm that maps to the 8Gb of the container agents (rule of thumb: Xmx+Xms = 1/4 of available memory), but on the highmeme instances that could be increased. Shouldn't we remove these options and let the JVM deal with the available memory instead (I mean: even with "old" JDK8 builds", the container cgroup memory is now handled correctly without OOM)

yeah once we merge this I plan to try out a PR to see if it does anything.

timja · 2021-10-22T08:54:11Z

I plan to ship this if it works, we can debate style later, given the master build is broken right now...

dduportal · 2021-10-22T08:56:10Z

I plan to ship this if it works, we can debate style later, given the master build is broken right now...

cc @jglick when it will be daylight for you: don't hesitate to comment this PR, even if merged: I'll take care of adding a new PR with any style/feedbacks.

lemeurherve approved these changes Oct 21, 2021

View reviewed changes

dduportal marked this pull request as draft October 21, 2021 13:06

dduportal mentioned this pull request Oct 21, 2021

feat [infra] allow runMaven with an integer for JDK version jenkins-infra/pipeline-library#236

Merged

dduportal marked this pull request as ready for review October 21, 2021 13:23

timja approved these changes Oct 21, 2021

View reviewed changes

timja requested a review from jglick October 21, 2021 13:34

timja added the skip-changelog Should not be shown in the changelog label Oct 21, 2021

jglick requested changes Oct 21, 2021

View reviewed changes

Jenkinsfile Outdated Show resolved Hide resolved

Jenkinsfile Outdated Show resolved Hide resolved

Jenkinsfile Outdated Show resolved Hide resolved

Jenkinsfile Outdated Show resolved Hide resolved

Jenkinsfile Show resolved Hide resolved

dduportal mentioned this pull request Oct 21, 2021

feat [maven 8,11,17] bump maven to 3.8.3 jenkins-infra/docker-inbound-agents#9

Merged

dduportal requested a review from jglick October 21, 2021 16:17

jglick approved these changes Oct 21, 2021

View reviewed changes

Jenkinsfile Show resolved Hide resolved

jglick reviewed Oct 21, 2021

View reviewed changes

chore(Jeninsfile) Switch Maven commands to infra.runWithMaven

c37fa30

The goal is to benefit from the the shared library jenkins-infra/pipeline-library that manages agents and tools. Signed-off-by: Damien Duportal <[email protected]>

dduportal force-pushed the patch-1 branch from 4092ec3 to c37fa30 Compare October 22, 2021 08:40

dduportal changed the title ~~ath: use the pipeline library infra.runMaven~~ chore(Jeninsfile) Switch Maven commands to infra.runWithMaven Oct 22, 2021

timja reviewed Oct 22, 2021

View reviewed changes

timja approved these changes Oct 22, 2021

View reviewed changes

timja merged commit d738127 into jenkinsci:master Oct 22, 2021

dduportal deleted the patch-1 branch October 22, 2021 08:58

timja mentioned this pull request Oct 22, 2021

CI: Let java figure out memory ergonomics #5846

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(Jeninsfile) Switch Maven commands to infra.runWithMaven #5843

chore(Jeninsfile) Switch Maven commands to infra.runWithMaven #5843

dduportal commented Oct 21, 2021 •

edited

Loading

jglick left a comment

dduportal commented Oct 21, 2021

jglick left a comment

jglick left a comment

jglick Oct 21, 2021

dduportal Oct 21, 2021

dduportal commented Oct 21, 2021

dduportal commented Oct 22, 2021 •

edited

Loading

timja Oct 22, 2021

dduportal Oct 22, 2021

timja Oct 22, 2021

timja Oct 22, 2021

timja commented Oct 22, 2021

dduportal commented Oct 22, 2021

chore(Jeninsfile) Switch Maven commands to infra.runWithMaven #5843

chore(Jeninsfile) Switch Maven commands to infra.runWithMaven #5843

Conversation

dduportal commented Oct 21, 2021 • edited Loading

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

jglick left a comment

Choose a reason for hiding this comment

dduportal commented Oct 21, 2021

jglick left a comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

jglick Oct 21, 2021

Choose a reason for hiding this comment

dduportal Oct 21, 2021

Choose a reason for hiding this comment

dduportal commented Oct 21, 2021

dduportal commented Oct 22, 2021 • edited Loading

timja Oct 22, 2021

Choose a reason for hiding this comment

dduportal Oct 22, 2021

Choose a reason for hiding this comment

timja Oct 22, 2021

Choose a reason for hiding this comment

timja Oct 22, 2021

Choose a reason for hiding this comment

timja commented Oct 22, 2021

dduportal commented Oct 22, 2021

dduportal commented Oct 21, 2021 •

edited

Loading

dduportal commented Oct 22, 2021 •

edited

Loading