[JENKINS-14538] Move tool configuration to separate page #2140

amuniz · 2016-03-18T17:29:24Z

This PR wraps up #2011.

It fixes remaining tasks:

Basic changes done
Needs better icon (or rather a different icon for Configure Jenkins, a large gear may work);
Change package names
Fix tests

@jenkinsci/code-reviewers and @reviewbybees

Conflicts: core/src/main/resources/hudson/model/Computer/sidepanel.jelly

…ENKINS-14538

amuniz · 2016-03-19T11:49:07Z

This needs to be debugged because it's basically not working. For example, JDK installations are not persisted to disk, so they are all gone after a restart. Investigating...

ghost · 2016-03-19T13:41:49Z

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.

amuniz · 2016-03-20T10:07:43Z

core/src/main/java/jenkins/model/Jenkins.java

@@ -3354,8 +3354,6 @@ public synchronized void doConfigSubmit( StaplerRequest req, StaplerResponse rsp

            systemMessage = Util.nullify(req.getParameter("system_message"));

-            setJDKs(req.bindJSONToList(JDK.class, json.get("jdks")));


This call is no more needed as the JDKs list is set in a call to its descriptor configure in GlobalToolConfiguration.doConfigure. Moreover it was clearing out the JDKs list on each general configuration submit (because there is no such jdks attribute in the JSON now).

daniel-beck · 2016-03-20T20:53:40Z

Looks solid. No real findings. No +1 from me, as I'm co-author.

oleg-nenashev · 2016-03-20T21:57:29Z

core/src/main/resources/jenkins/tools/GlobalToolConfiguration/index.groovy

+    l.main_panel {
+        h1 {
+            l.icon(class: 'icon-setting icon-xlg')
+            // TODO more appropriate icon


Do you plan to address this TODO before the PR gets merged?

Well, I'm not a good icon designer. If someone sends one, I'll use it.
Anyway, now that Manage Jenkins is using icon-gear I think it's ok to keep icon-setting here.

This TODO is obsolete. Since this PR switches the Manage Jenkins icon, uses of this icon for tools is appropriate.

So 🐜 delete the misleading comment.

oleg-nenashev · 2016-03-20T21:58:22Z

looks good to me 👍

jglick · 2016-03-21T14:35:07Z

java.lang.NullPointerException: null
    at hudson.plugins.git.GitTool.onLoaded(GitTool.java:135)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:606)
    at hudson.init.TaskMethodFinder.invoke(TaskMethodFinder.java:106)
    at hudson.init.TaskMethodFinder$TaskImpl.run(TaskMethodFinder.java:175)
    at org.jvnet.hudson.reactor.Reactor.runTask(Reactor.java:282)
    at jenkins.model.Jenkins$8.runTask(Jenkins.java:1009)

might be yours.

amuniz · 2016-03-21T14:37:46Z

mvn -f test test -Dtest=DescriptorTest#overriddenId passed locally, trying to see why it fails in the PR build.

amuniz · 2016-03-21T14:43:19Z

It seems the root cause is git-plugin not being installed correctly:

SEVERE: Failed to install Git client plugin
java.io.IOException: Failed to dynamically deploy this plugin
    at hudson.model.UpdateCenter$InstallationJob._run(UpdateCenter.java:1648)
    at hudson.model.UpdateCenter$DownloadJob.run(UpdateCenter.java:1426)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
    at java.util.concurrent.FutureTask.run(FutureTask.java:262)
    at hudson.remoting.AtmostOneThreadExecutor$Worker.run(AtmostOneThreadExecutor.java:110)
    at java.lang.Thread.run(Thread.java:745)
Caused by: java.io.IOException: Failed to install git-client plugin
    at hudson.PluginManager.dynamicLoad(PluginManager.java:698)
    at hudson.model.UpdateCenter$InstallationJob._run(UpdateCenter.java:1644)
    ... 5 more
Caused by: java.lang.NullPointerException
    at hudson.PluginManager.dynamicLoad(PluginManager.java:693)
    ... 6 more

I'm going to close/reopen to get a new build.

amuniz · 2016-03-21T16:03:27Z

This time DescriptorTest passed (one skipped because of @Ignore):

Running hudson.model.DescriptorTest
Tests run: 4, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 76.092 sec - in hudson.model.DescriptorTest

But hudson.LauncherTest.remoteKill failed. It seems just flaky tests failing sporadically.

jglick · 2016-03-21T17:34:04Z

core/src/main/java/jenkins/model/GlobalConfigurationCategory.java

@@ -73,11 +71,12 @@ public String getDisplayName() {
    public static class Security extends GlobalConfigurationCategory {
        @Override
        public String getShortDescription() {
-            return Messages.GlobalSecurityConfiguration_Description();
+            return hudson.security.Messages.GlobalSecurityConfiguration_Description();


BTW not an issue introduced in this PR, but would be nice at some point to use core/move-l10n.groovy to put this message in the natural place.

jglick · 2016-03-21T17:43:49Z

🐝

daniel-beck and others added 4 commits February 3, 2016 18:21

[FIX JENKINS-14538] Move tool configuration to separate page

ea5b2a7

Use gear icon for Configure to free tool icon for Configure Tools

4184b5a

Merge branch '2.0' of github.com:jenkinsci/jenkins into JENKINS-14538

03291cf

Conflicts: core/src/main/resources/hudson/model/Computer/sidepanel.jelly

Merge branch 'JENKINS-14538' of github.com:daniel-beck/jenkins into J…

9e66786

…ENKINS-14538

daniel-beck mentioned this pull request Mar 19, 2016

[FIXED JENKINS-14538] Move tool configuration to separate page #2011

Closed

4 tasks

daniel-beck added the work-in-progress The PR is under active development, not ready to the final review label Mar 19, 2016

amuniz added 5 commits March 19, 2016 13:49

Save tools configuration after configure

6f8bf46

Fix Maven tool configuration test

288796a

Fix JDK tool installer test

54ba3c1

Moved to jenkins.tools package

807fa9a

Reuse local variable

4f7c7de

daniel-beck added needs-more-reviews Complex change, which would benefit from more eyes and removed work-in-progress The PR is under active development, not ready to the final review labels Mar 19, 2016

JDKs were not loaded from disk

81c8f15

amuniz reviewed Mar 20, 2016
View reviewed changes

oleg-nenashev reviewed Mar 20, 2016
View reviewed changes

API use restricted and simplified logic in Functions

7045be8

amuniz closed this Mar 21, 2016

amuniz reopened this Mar 21, 2016

jglick reviewed Mar 21, 2016
View reviewed changes

daniel-beck merged commit 82d3eb8 into jenkinsci:2.0 Mar 23, 2016

daniel-beck mentioned this pull request Jul 6, 2022

[JENKINS-68282] New design for configure project page #6485

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-14538] Move tool configuration to separate page #2140

[JENKINS-14538] Move tool configuration to separate page #2140

amuniz commented Mar 18, 2016

amuniz commented Mar 19, 2016

ghost commented Mar 19, 2016

amuniz Mar 20, 2016

jglick Mar 21, 2016

daniel-beck commented Mar 20, 2016

oleg-nenashev Mar 20, 2016

amuniz Mar 20, 2016

daniel-beck Mar 20, 2016

jglick Mar 21, 2016

oleg-nenashev commented Mar 20, 2016

jglick commented Mar 21, 2016

amuniz commented Mar 21, 2016

amuniz commented Mar 21, 2016

amuniz commented Mar 21, 2016

jglick Mar 21, 2016

jglick commented Mar 21, 2016

		@@ -3354,8 +3354,6 @@ public synchronized void doConfigSubmit( StaplerRequest req, StaplerResponse rsp

		systemMessage = Util.nullify(req.getParameter("system_message"));

		setJDKs(req.bindJSONToList(JDK.class, json.get("jdks")));

[JENKINS-14538] Move tool configuration to separate page #2140

[JENKINS-14538] Move tool configuration to separate page #2140

Conversation

amuniz commented Mar 18, 2016

amuniz commented Mar 19, 2016

ghost commented Mar 19, 2016

amuniz Mar 20, 2016

Choose a reason for hiding this comment

jglick Mar 21, 2016

Choose a reason for hiding this comment

daniel-beck commented Mar 20, 2016

oleg-nenashev Mar 20, 2016

Choose a reason for hiding this comment

amuniz Mar 20, 2016

Choose a reason for hiding this comment

daniel-beck Mar 20, 2016

Choose a reason for hiding this comment

jglick Mar 21, 2016

Choose a reason for hiding this comment

oleg-nenashev commented Mar 20, 2016

jglick commented Mar 21, 2016

amuniz commented Mar 21, 2016

amuniz commented Mar 21, 2016

amuniz commented Mar 21, 2016

jglick Mar 21, 2016

Choose a reason for hiding this comment

jglick commented Mar 21, 2016