[JENKINS-26100] Switch buildEnvVars and others to Run #492

abayer · 2017-05-01T17:29:54Z

cc @reviewbybees esp @MarkEWaite @jglick

ghost · 2017-05-01T18:24:52Z

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.

jglick

OK except for requested buildEnvVars rename.

jglick · 2017-05-01T22:03:51Z

pom.xml

@@ -24,7 +24,8 @@
  <inceptionYear>2007</inceptionYear>

  <properties>
-    <jenkins.version>1.625.3</jenkins.version>
+    <jenkins-core.version>2.58-20170501.171905-3</jenkins-core.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->
+    <jenkins-war.version>2.58-20170501.171927-3</jenkins-war.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->
    <java.level>7</java.level>


jglick · 2017-05-01T22:04:52Z

src/main/java/jenkins/plugins/git/AbstractGitSCMSource.java

@@ -470,7 +470,7 @@ protected String getCacheEntry() {
    }

    protected static File getCacheDir(String cacheEntry) {
-        Jenkins jenkins = Jenkins.getInstance();
+        Jenkins jenkins = Jenkins.getInstanceOrNull();


Most of these should actually be getInstance() with no null check, but good enough for now.

I went for the most stupidly non-logic-altering Findbugs fix I could. =) Wasn't 100% sure there weren't some cases where they might be expecting the existing behavior with a possibly null jenkins instance, so hey, better safe than sorry.

jglick · 2017-05-03T19:49:20Z

Hudson Parameterized Trigger plugin v2.4 failed to load.
 - subversion v1.7 is missing. To fix, install v1.7 or later.

Not your fault, just a sign that this plugin’s Jenkinsfile is missing the usual idiom to verify that its tests run against Jenkins 2.12+, e.g.:

buildPlugin(jenkinsVersions: [null, '2.32.3'])

MarkEWaite · 2017-05-03T20:00:18Z

Last time I enabled the plugin compat tests, it failed because the combination of tests and compilation took more than an hour to complete. The ci.jenkins.io jobs (correctly) limit themselves to not more than an hour elapsed time for the total job.

Plugin compat testing is enabled again, and we'll see if it performs better now.

jglick · 2017-05-03T20:52:11Z

But jenkinsVersions will be run in parallel, and the timeout is wall clock time, so why would it matter?

MarkEWaite · 2017-05-04T13:15:01Z

With plugin compat enabled, it now does not timeout (maybe a false recollection on my part). It reports findbugs warnings on the 2.46.1 build which it does not report on other builds. Unfortunately, my first attempts to duplicate that result locally have failed. My findbugs is silent, even when I set jenkins.version=2.46.1.

My second attempt shows how to duplicate the problem. I mistakenly used -DskipTests, forgetting that it excludes findbugs runs for speed. If I run with at least one test, or with -Dtest=xyzzy -DfailIfNoTests=false, then I can see the findbugs output when compiling with 2.46.1.

I suspect I won't have time to explore fixes until after the end of the working day today.

Short term, I've stopped failing on findbugs warning messages. See daf202d

jglick · 2017-05-04T13:27:22Z

The most common introduced FB error is due to @stephenc’s messing around with Jenkins.getInstance. Either switch to getActiveInstance, if appropriate, or leave the getInstance call with a null check and add

@SuppressFBWarnings(value="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE", justification="TODO 1.653+ switch to Jenkins.getInstanceOrNull")

Yes this is obnoxious which is why I asked him to leave existing methods alone, and rely on deprecations if necessary, but he insisted…

jglick · 2017-05-04T13:28:12Z

FTR my NetBeans action binding for checking FindBugs:

mvn -DskipTests=false clean compile findbugs:check

MarkEWaite · 2017-05-04T13:34:03Z

I'm not sure that action binding will work with the git plugin's current pom. As far as I can tell, once -DskipTests=false is passed as an argument, then findbugs:check is a no-op. I think that is reasonable (if you're skipping tests, you probably don't want to run findbugs), but it surprised me in this case.

My solution was -Dtest=CGitTest findbugs:check findbugs:gui. Runs a fast test, and reports findbugs results.

jglick · 2017-05-04T13:41:32Z

This works in other plugins. FindBugs is skipped only if -DskipTests or -DskipTests=true is used. Omitting, or passing -DskipTests=false, runs FindBugs.

MarkEWaite · 2017-05-04T13:46:09Z

Thanks for the education! Inattentional blindness problem on my part. I saw the -DskipTests= in your action binding and didn't look at the false value. Nice technique.

jglick · 2017-05-16T22:59:30Z

pom.xml

-    <jenkins.version>1.625.3</jenkins.version>
-    <java.level>7</java.level>
+    <jenkins-core.version>2.58-20170502.192524-8</jenkins-core.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->
+    <jenkins-war.version>2.58-20170502.192544-8</jenkins-war.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->


2.60 I meant.

jglick · 2017-05-18T22:50:12Z

src/main/java/hudson/plugins/git/GitChangeSet.java

-        Hudson hudson = Hudson.getInstance();
-        if (hudson == null) {
+        Jenkins jenkins = Jenkins.getInstance();
+        if (jenkins == null) {


Maybe just revert changes in this file?

jglick · 2017-05-18T22:50:27Z

src/main/java/hudson/plugins/git/GitSCM.java

+        buildEnvironment(build, env);
+    }
+
+    public void buildEnvironment(Run<?, ?> build, java.util.Map<String, String> env) {


Put in the usual TODOs.

Conflicts: pom.xml

Letty-Liang · 2017-05-24T09:54:21Z

looking forward to be published soon

====
With this pr, it seems that we still can't get git properties in env while using pipeline jobs.

Conflicts: pom.xml

abayer · 2017-06-06T15:15:59Z

@reviewbybees done

@MarkEWaite Can I persuade you to merge this? =)

MarkEWaite · 2017-06-06T18:03:08Z

It will be several days (at least) before I have a chance to review it. Can the merge wait until next week? What if it needs to wait another week beyond that?

abayer · 2017-06-06T18:05:51Z

@MarkEWaite No rush at all - just wanted to make sure this wasn't lost.

jglick · 2017-06-16T12:15:38Z

pom.xml

      <optional>true</optional>
+<<<<<<< HEAD
+   </dependency>
+=======


Bad merge conflict resolution!

Conflicts: pom.xml

jglick

:shipit:

abayer · 2017-06-20T16:30:30Z

@MarkEWaite Any news? =)

MarkEWaite · 2017-06-20T17:59:52Z

@abayer, sorry, but no progress on this pull request yet. Is it urgent, or blocking other work that I should place it ahead of other things in my queue (like the submodule checkout regression or the credential exclusion issue)?

[JENKINS-26100] Switch buildEnvVars and others to Run

47f738c

abayer requested review from jglick and MarkEWaite May 1, 2017 17:29

abayer mentioned this pull request May 1, 2017

[JENKINS-26100] Return a map of SCM-contributed variables jenkinsci/workflow-scm-step-plugin#16

Merged

Make findbugs happy.

6ee22da

jglick approved these changes May 1, 2017

View reviewed changes

Update to buildEnvironment.

803d0cb

jglick approved these changes May 3, 2017

View reviewed changes

abayer mentioned this pull request May 4, 2017

[JENKINS-24141] Pull ChangeLogSet-related logic out of AbstractBuild jenkinsci/jenkins#2730

Merged

3 tasks

abayer added the work-in-progress label May 4, 2017

jglick reviewed May 16, 2017

View reviewed changes

Switch to release

e9dbe68

abayer removed the work-in-progress label May 18, 2017

abayer changed the title ~~NOT READY FOR MERGE [JENKINS-26100] Switch buildEnvVars and others to Run~~ [JENKINS-26100] Switch buildEnvVars and others to Run May 18, 2017

Reverting back to 1.625.3 baseline

3c125fb

jglick approved these changes May 18, 2017

View reviewed changes

abayer added 2 commits May 19, 2017 11:39

Another bit of review comments

b409573

Clarification

6d158dc

jglick approved these changes May 19, 2017

View reviewed changes

Merge remote-tracking branch 'origin' into jenkins-26100

3681660

Conflicts: pom.xml

Switch to modern parameterized-trigger dependency.

74cd756

Merge remote-tracking branch 'origin/master' into jenkins-26100

f4bb730

Conflicts: pom.xml

jglick requested changes Jun 16, 2017

View reviewed changes

pom.xml Outdated

<optional>true</optional>

<<<<<<< HEAD

</dependency>

=======

Copy link

Member

jglick Jun 16, 2017

Choose a reason for hiding this comment

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

Bad merge conflict resolution!

abayer added 2 commits June 16, 2017 09:53

Whoops, fixing merge bork.

e38f561

Merge remote-tracking branch 'origin/master' into jenkins-26100

133609e

Conflicts: pom.xml

jglick approved these changes Jun 16, 2017

View reviewed changes

MarkEWaite approved these changes Jun 21, 2017

View reviewed changes

MarkEWaite merged commit 907253d into jenkinsci:master Jun 21, 2017

MarkEWaite added the bugfix Fixes a bug - used by Release Drafter label Jul 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-26100] Switch buildEnvVars and others to Run #492

[JENKINS-26100] Switch buildEnvVars and others to Run #492

abayer commented May 1, 2017 •

edited

Loading

ghost commented May 1, 2017

jglick left a comment

jglick May 1, 2017

jglick May 1, 2017

abayer May 1, 2017

jglick commented May 3, 2017

MarkEWaite commented May 3, 2017 •

edited

Loading

jglick commented May 3, 2017

MarkEWaite commented May 4, 2017 •

edited

Loading

jglick commented May 4, 2017

jglick commented May 4, 2017

MarkEWaite commented May 4, 2017

jglick commented May 4, 2017

MarkEWaite commented May 4, 2017

jglick May 16, 2017

jglick May 16, 2017

jglick May 18, 2017

abayer May 19, 2017

jglick May 18, 2017

abayer May 19, 2017

Letty-Liang commented May 24, 2017 •

edited

Loading

abayer commented Jun 6, 2017

MarkEWaite commented Jun 6, 2017

abayer commented Jun 6, 2017

jglick Jun 16, 2017

jglick left a comment •

edited

Loading

abayer commented Jun 20, 2017

MarkEWaite commented Jun 20, 2017

[JENKINS-26100] Switch buildEnvVars and others to Run #492

[JENKINS-26100] Switch buildEnvVars and others to Run #492

Conversation

abayer commented May 1, 2017 • edited Loading

ghost commented May 1, 2017

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented May 3, 2017

MarkEWaite commented May 3, 2017 • edited Loading

jglick commented May 3, 2017

MarkEWaite commented May 4, 2017 • edited Loading

jglick commented May 4, 2017

jglick commented May 4, 2017

MarkEWaite commented May 4, 2017

jglick commented May 4, 2017

MarkEWaite commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Letty-Liang commented May 24, 2017 • edited Loading

abayer commented Jun 6, 2017

MarkEWaite commented Jun 6, 2017

abayer commented Jun 6, 2017

Choose a reason for hiding this comment

jglick left a comment • edited Loading

Choose a reason for hiding this comment

abayer commented Jun 20, 2017

MarkEWaite commented Jun 20, 2017

abayer commented May 1, 2017 •

edited

Loading

MarkEWaite commented May 3, 2017 •

edited

Loading

MarkEWaite commented May 4, 2017 •

edited

Loading

Letty-Liang commented May 24, 2017 •

edited

Loading

jglick left a comment •

edited

Loading