Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Mar 23, 2016
9 changes: 2 additions & 7 deletions core/src/main/java/hudson/Functions.java
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@

import jenkins.model.GlobalConfiguration;
import jenkins.model.GlobalConfigurationCategory;
import jenkins.model.GlobalConfigurationCategory.Unclassified;
import jenkins.model.Jenkins;
import jenkins.model.ModelObjectWithChildren;
import jenkins.model.ModelObjectWithContextMenu;
Expand Down Expand Up @@ -965,12 +964,8 @@ public static Collection<Descriptor> getSortedDescriptorsForGlobalConfig(Predica
Descriptor d = c.getInstance();
if (d.getGlobalConfigPage()==null) continue;

if (d instanceof GlobalConfiguration) {
if (predicate.apply(((GlobalConfiguration)d).getCategory()))
r.add(new Tag(c.ordinal(), d));
} else {
if (predicate.apply(GlobalConfigurationCategory.get(Unclassified.class)))
r.add(new Tag(0, d));
if (predicate.apply(d.getCategory())) {
r.add(new Tag(c.ordinal(), d));
}
}
Collections.sort(r);
Expand Down
15 changes: 14 additions & 1 deletion core/src/main/java/hudson/model/Descriptor.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import hudson.util.ReflectionUtils;
import hudson.util.ReflectionUtils.Parameter;
import hudson.views.ListViewColumn;
import jenkins.model.GlobalConfiguration;
import jenkins.model.GlobalConfigurationCategory;
import jenkins.model.Jenkins;
import net.sf.json.JSONArray;
import net.sf.json.JSONObject;
Expand Down Expand Up @@ -807,7 +809,18 @@ public String getConfigPage() {
public String getGlobalConfigPage() {
return getViewPage(clazz, getPossibleViewNames("global"), null);
}


/**
* Define the global configuration category the global config of this Descriptor is in.
*
* @return never null, always the same value for a given instance of {@link Descriptor}.
*
* @since TODO, used to be in {@link GlobalConfiguration} before that.
*/
public GlobalConfigurationCategory getCategory() {
return GlobalConfigurationCategory.get(GlobalConfigurationCategory.Unclassified.class);
}

private String getViewPage(Class<?> clazz, String pageName, String defaultValue) {
return getViewPage(clazz,Collections.singleton(pageName),defaultValue);
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/model/ManageJenkinsAction.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
public class ManageJenkinsAction implements RootAction {
public String getIconFileName() {
if (Jenkins.getInstance().hasPermission(Jenkins.ADMINISTER))
return "setting.png";
return "gear2.png";
else
return null;
}
Expand Down
9 changes: 9 additions & 0 deletions core/src/main/java/hudson/tools/ToolDescriptor.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@
import java.lang.reflect.Type;
import java.util.Collections;
import java.util.List;

import jenkins.model.GlobalConfigurationCategory;
import jenkins.model.Jenkins;
import jenkins.tools.ToolConfigurationCategory;
import net.sf.json.JSONObject;
import org.jvnet.tiger_types.Types;
import org.kohsuke.stapler.QueryParameter;
Expand Down Expand Up @@ -89,6 +92,12 @@ public List<ToolPropertyDescriptor> getPropertyDescriptors() {
return PropertyDescriptor.for_(ToolProperty.all(),clazz);
}


@Override
public GlobalConfigurationCategory getCategory() {
return GlobalConfigurationCategory.get(ToolConfigurationCategory.class);
}

/**
* Optional list of installers to be configured by default for new tools of this type.
* If there are popular versions of the tool available using generic installation techniques,
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/jenkins/management/ConfigureLink.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public class ConfigureLink extends ManagementLink {

@Override
public String getIconFileName() {
return "setting.png";
return "gear2.png";
}

public String getDisplayName() {
Expand Down
9 changes: 0 additions & 9 deletions core/src/main/java/jenkins/model/GlobalConfiguration.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,6 @@ public final Descriptor<GlobalConfiguration> getDescriptor() {
return this;
}

/**
* Every {@link GlobalConfiguration} belongs to a specific category.
*
* @return never null, always the same value for a given instance of {@link GlobalConfiguration}.
*/
public GlobalConfigurationCategory getCategory() {
return GlobalConfigurationCategory.get(GlobalConfigurationCategory.Unclassified.class);
}

@Override
public String getGlobalConfigPage() {
return getConfigPage();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
import hudson.ExtensionList;
import hudson.ExtensionPoint;
import hudson.model.ModelObject;
import hudson.security.*;
import hudson.security.Messages;

/**
* Grouping of related {@link GlobalConfiguration}s.
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

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

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.

}

public String getDisplayName() {
return hudson.security.Messages.GlobalSecurityConfiguration_DisplayName();
}
}

}
2 changes: 0 additions & 2 deletions core/src/main/java/jenkins/model/Jenkins.java
Original file line number Diff line number Diff line change
Expand Up @@ -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")));

boolean result = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good.

for (Descriptor<?> d : Functions.getSortedDescriptorsForGlobalConfigUnclassified())
result &= configureDescriptor(req,json,d);
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/jenkins/mvn/GlobalMavenConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import hudson.Extension;
import jenkins.model.GlobalConfiguration;
import jenkins.model.GlobalConfigurationCategory;
import jenkins.tools.ToolConfigurationCategory;
import net.sf.json.JSONObject;

import org.kohsuke.stapler.StaplerRequest;
Expand All @@ -16,6 +18,11 @@ public GlobalMavenConfig() {
load();
}

@Override
public ToolConfigurationCategory getCategory() {
return GlobalConfigurationCategory.get(ToolConfigurationCategory.class);
Copy link
Member

Choose a reason for hiding this comment

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

So I think this is the potential argument against ToolConfigurationCategory being in jenkins.tools—that it might be used by things which are not actually ToolDescriptors but something conceptually related. On the other hand I would expect that any plugin which is defining such configuration would also be defining ToolDescriptors and thus implicitly using ToolConfigurationCategory.

}

@Override
public boolean configure(StaplerRequest req, JSONObject json) throws FormException {
req.bindJSON(this, json);
Expand Down
109 changes: 109 additions & 0 deletions core/src/main/java/jenkins/tools/GlobalToolConfiguration.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* The MIT License
*
* Copyright (c) 2016 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
package jenkins.tools;

import com.google.common.base.Predicate;
import hudson.Extension;
import hudson.Functions;
import hudson.model.Descriptor;
import hudson.model.ManagementLink;
import hudson.security.Permission;
import hudson.util.FormApply;
import jenkins.model.GlobalConfigurationCategory;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;

import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;

import javax.servlet.ServletException;
import java.io.IOException;
import java.util.logging.Level;
import java.util.logging.Logger;

@Extension(ordinal = Integer.MAX_VALUE - 220)
Copy link
Member

Choose a reason for hiding this comment

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

How about @Restricted(NoExternalUse.class)?

Copy link
Member

Choose a reason for hiding this comment

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

+1. Makes sense for all internal extension point implementations

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@Restricted(NoExternalUse.class)
public class GlobalToolConfiguration extends ManagementLink {

@Override
public String getIconFileName() {
return "setting.png";
}

@Override
public String getDisplayName() {
return jenkins.management.Messages.ConfigureTools_DisplayName();
}

@Override
public String getDescription() {
return jenkins.management.Messages.ConfigureTools_Description();
Copy link
Member

Choose a reason for hiding this comment

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

🐜 should this not be in jenkins.tools.Messages, since both of its usages are in that package?

}

@Override
public String getUrlName() {
return "configureTools";
}

@Override
public Permission getRequiredPermission() {
return Jenkins.ADMINISTER;
}

public synchronized void doConfigure(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException, Descriptor.FormException {
boolean result = configure(req, req.getSubmittedForm());
LOGGER.log(Level.FINE, "tools saved: "+result);
FormApply.success(req.getContextPath() + "/manage").generateResponse(req, rsp, null);
}

private boolean configure(StaplerRequest req, JSONObject json) throws hudson.model.Descriptor.FormException, IOException {
Jenkins j = Jenkins.getInstance();
j.checkPermission(Jenkins.ADMINISTER);

boolean result = true;
for(Descriptor<?> d : Functions.getSortedDescriptorsForGlobalConfig(FILTER)){
result &= configureDescriptor(req, json, d);
}
j.save();

return result;
}

private boolean configureDescriptor(StaplerRequest req, JSONObject json, Descriptor<?> d) throws Descriptor.FormException {
String name = d.getJsonSafeClassName();
JSONObject js = json.has(name) ? json.getJSONObject(name) : new JSONObject(); // if it doesn't have the property, the method returns invalid null object.
json.putAll(js);
return d.configure(req, js);
Copy link
Member

Choose a reason for hiding this comment

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

🐜 copy-pasted code should perhaps be factored into a utility class for creating standard kinds of global configuration screens.

}

public static Predicate<GlobalConfigurationCategory> FILTER = new Predicate<GlobalConfigurationCategory>() {
public boolean apply(GlobalConfigurationCategory input) {
return input instanceof ToolConfigurationCategory;
}
};

private static final Logger LOGGER = Logger.getLogger(GlobalToolConfiguration.class.getName());
}
21 changes: 21 additions & 0 deletions core/src/main/java/jenkins/tools/ToolConfigurationCategory.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package jenkins.tools;
Copy link
Member

Choose a reason for hiding this comment

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

Tempted to make this @Restricted(NoExternalUse.class) for a time to first determine how this should be used by plugins. What kinds of configuration should be moved here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if I follow. @Restricted can not be used in a package.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW this is actually not usable from plugins, they just have to implement ToolInstallation and declare a ToolDescriptor.

Copy link
Member

Choose a reason for hiding this comment

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

This comment was supposed to go on line 12(ish). The idea is to prevent plugins from declaring whatever Descriptor they have to be in the tools category (like we do for GlobalMavenConfig).

Copy link
Member

Choose a reason for hiding this comment

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

But GlobalMavenConfig is precisely the example of why this should not be restricted: doing so would prevent us from implementing JENKINS-26462, unless we moved that configuration back into the generic global configuration screen.


import hudson.Extension;
import jenkins.model.GlobalConfigurationCategory;

/**
* Global configuration of tool locations and installers.
*
* @since 2.0
*/
@Extension
public class ToolConfigurationCategory extends GlobalConfigurationCategory {
@Override
public String getShortDescription() {
return jenkins.management.Messages.ConfigureTools_Description();
}

public String getDisplayName() {
return jenkins.management.Messages.ConfigureTools_DisplayName();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ THE SOFTWARE.
<l:tasks>
<l:task href=".." icon="icon-up icon-md" title="${%Back to Loggers}"/>
<l:task href="." icon="icon-notepad icon-md" title="${%Log records}"/>
<l:task href="configure" icon="icon-setting icon-md" title="${%Configure}"/>
<l:task href="configure" icon="icon-gear2 icon-md" title="${%Configure}"/>
<l:task href="delete" icon="icon-edit-delete icon-md" title="${%Delete}"/>
</l:tasks>
</l:side-panel>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ THE SOFTWARE.
<j:forEach var="lr" items="${it.logRecorders.values()}">
<local:row name="${lr.name}" href="${lr.searchUrl}/">
<a href="${lr.searchUrl}/configure">
<l:icon class="icon-setting icon-lg" tooltip="Configure"/>
<l:icon class="icon-gear2 icon-lg" tooltip="Configure"/>
</a>
</local:row>
</j:forEach>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ THE SOFTWARE.
<l:side-panel>
<l:tasks>
<l:task href="${rootURL}/" icon="icon-up icon-md" title="${%Back to Dashboard}"/>
<l:task href="${rootURL}/manage" icon="icon-setting icon-md" title="${%Manage Jenkins}"/>
<l:task href="${rootURL}/manage" icon="icon-gear2 icon-md" title="${%Manage Jenkins}"/>
<l:task href="." icon="icon-clipboard icon-md" title="${%Logger List}"/>
<l:task href="all" icon="icon-notepad icon-md" title="${%All Logs}"/>
<l:task href="new" icon="icon-new-package icon-md" title="${%New Log Recorder}"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ THE SOFTWARE.
<td><!-- config link -->
<j:if test="${c.hasPermission(c.CONFIGURE)}">
<a href="${rootURL}/${c.url}configure">
<l:icon class="icon-setting icon-lg" tooltip="${%Configure}"/>
<l:icon class="icon-gear2 icon-lg" tooltip="${%Configure}"/>
</a>
</j:if>
</td>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ THE SOFTWARE.
<j:getStatic var="createPermission" className="hudson.model.Computer" field="CREATE"/>
<l:tasks>
<l:task href="${rootURL}/" icon="icon-up icon-md" title="${%Back to Dashboard}"/>
<l:task href="${rootURL}/manage" icon="icon-setting icon-md" permission="${app.ADMINISTER}" title="${%Manage Jenkins}"/>
<l:task href="${rootURL}/manage" icon="icon-gear2 icon-md" permission="${app.ADMINISTER}" title="${%Manage Jenkins}"/>
<l:task href="new" icon="icon-new-computer icon-md" permission="${createPermission}" title="${%New Node}"/>
<l:task href="configure" icon="icon-setting icon-md" permission="${app.ADMINISTER}" title="${%Configure}"/>
<l:task href="configure" icon="icon-gear2 icon-md" permission="${app.ADMINISTER}" title="${%Configure}"/>
</l:tasks>
<t:queue items="${app.queue.items}" />
<t:executors />
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/resources/hudson/model/Label/sidepanel.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ THE SOFTWARE.
<l:task contextMenu="false" href="${rootURL}/" icon="icon-up icon-md" title="${%Back to Dashboard}"/>
<l:task contextMenu="false" href="${url}" icon="icon-attribute icon-md" title="${%Overview}"/>
<j:if test="${it.atom}">
<l:task href="${url}/configure" icon="icon-setting icon-md" title="${%Configure}" permission="${app.ADMINISTER}" />
<l:task href="${url}/configure" icon="icon-gear2 icon-md" title="${%Configure}" permission="${app.ADMINISTER}" />
</j:if>
<l:task href="${url}/load-statistics" icon="icon-monitor icon-md" title="${%Load Statistics}"/>
<st:include page="actions.jelly" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ THE SOFTWARE.
<l:tasks>
<l:task href="${rootURL}/" icon="icon-up icon-md" title="${%Back to Dashboard}"/>
<l:isAdmin>
<l:task href="${rootURL}/manage" icon="icon-setting icon-md" title="${%Manage Jenkins}"/>
<l:task href="${rootURL}/manage" icon="icon-gear2 icon-md" title="${%Manage Jenkins}"/>
<l:task href="${rootURL}/pluginManager/" icon="icon-plugin icon-lg" title="${%Manage Plugins}"/>
</l:isAdmin>
</l:tasks>
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/resources/hudson/model/User/sidepanel.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ THE SOFTWARE.
<l:task contextMenu="false" href="${rootURL}/asynchPeople/" icon="icon-up icon-md" title="${%People}"/>
<l:task contextMenu="false" href="${rootURL}/${it.url}/" icon="icon-search icon-md" title="${%Status}"/>
<l:task href="${rootURL}/${it.url}/builds" icon="icon-notepad icon-md" title="${%Builds}"/>
<l:task href="${rootURL}/${it.url}/configure" icon="icon-setting icon-md" permission="${app.ADMINISTER}" title="${%Configure}"/>
<l:task href="${rootURL}/${it.url}/configure" icon="icon-gear2 icon-md" permission="${app.ADMINISTER}" title="${%Configure}"/>
<t:actions actions="${it.propertyActions}"/>
<t:actions actions="${it.transientActions}"/>
<j:if test="${it.canDelete()}">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ THE SOFTWARE.
<td><a href="${user.url}/">${user.id}</a></td>
<td><a href="${user.url}/">${user}</a></td>
<td>
<a href="${user.url}/configure"><l:icon class="icon-setting icon-lg"/></a>
<a href="${user.url}/configure"><l:icon class="icon-gear2 icon-lg"/></a>
<j:if test="${user.canDelete()}">
<a href="${user.url}/delete"><l:icon class="icon-edit-delete icon-md"/></a>
</j:if>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ THE SOFTWARE.
<l:side-panel>
<l:tasks>
<l:task href="${rootURL}/" icon="icon-up icon-md" title="${%Back to Dashboard}"/>
<l:task href="${rootURL}/manage" icon="icon-setting icon-md" permission="${app.ADMINISTER}" title="${%Manage Jenkins}"/>
<l:task href="${rootURL}/manage" icon="icon-gear2 icon-md" permission="${app.ADMINISTER}" title="${%Manage Jenkins}"/>
<l:task href="addUser" icon="icon-new-user icon-md" permission="${app.ADMINISTER}" title="${%Create User}"/>
</l:tasks>
</l:side-panel>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
ConfigureLink.DisplayName=Configure System
ConfigureLink.Description=Configure global settings and paths.

ConfigureTools.DisplayName=Global Tool Configuration
ConfigureTools.Description=Configure tools, their locations and automatic installers.

ReloadLink.DisplayName=Reload Configuration from Disk
ReloadLink.Description=Discard all the loaded data in memory and reload everything from file system.\n\
Useful when you modified config files directly on disk.
Expand Down
Loading