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-70729] Rework clouds management into multiple pages #7658

Merged
merged 56 commits into from
Apr 28, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
6da6aa1
Rework clouds management into multiple pages
Vlatombe Jan 26, 2023
2551c94
Fix broken test + spotbugs
Vlatombe Feb 17, 2023
a09ec32
Use one column layout
timja Mar 4, 2023
082f650
Progress
timja Mar 4, 2023
ed9117c
Fix azure plugin
timja Mar 4, 2023
fe0315e
Remove redundant permission check
timja Mar 4, 2023
6dc25b1
Improve icon accessibility
timja Mar 4, 2023
72fe6a8
Clouds compatibility
Vlatombe Mar 4, 2023
8254ed9
Merge branch 'master' into manage-clouds
timja Mar 4, 2023
a9be4d0
Consistent casing and titles across cloud pages
timja Mar 4, 2023
b66695b
Refactor empty state handling
timja Mar 5, 2023
6d9bc85
Address review comments
timja Mar 5, 2023
ffac4a7
Fix Nodes configure link not being marked as active
timja Mar 5, 2023
c873c17
Fixups
timja Mar 5, 2023
db8227b
Updating links to "configureClouds" to go to "cloudSet"
Vlatombe Mar 6, 2023
057bab5
Deprecating GlobalCloudConfiguration
Vlatombe Mar 6, 2023
730d593
Removing missing sidepanel, and layout is one-column anyway
Vlatombe Mar 6, 2023
c6b26d0
Remove dead code
timja Mar 6, 2023
03c2672
Add trailing / to prevent unneeded redirect
timja Mar 6, 2023
82aaed3
Restore action box
Vlatombe Mar 6, 2023
e9d5d62
Update core/src/main/resources/jenkins/agents/CloudSet/index.jelly
timja Mar 6, 2023
ff3b1c6
Form apply support
Vlatombe Mar 6, 2023
c72b4f5
Revert back to isAdmin check
Vlatombe Mar 6, 2023
71638cc
Use /cloud instead of /cloudSet and keep navigation consistent within…
Vlatombe Mar 8, 2023
788ce99
Fix tests
Vlatombe Mar 8, 2023
0f6e0ad
Update missed reference
timja Mar 9, 2023
6d54cac
Update core/src/main/resources/hudson/model/ComputerSet/sidepanel.jelly
timja Mar 10, 2023
498b469
Merge branch 'master' into manage-clouds
timja Mar 10, 2023
4dbd2fd
Adjust content after management links shortened
timja Mar 10, 2023
5908c1d
Add clouds link
timja Mar 10, 2023
125ce33
Merge branch 'master' into manage-clouds
Vlatombe Mar 30, 2023
e0944c7
Cloud#reconfigure doesn't need to be public
Vlatombe Mar 30, 2023
3dc86ed
Add license header, remove StaplerFallback
Vlatombe Mar 30, 2023
4d4f2a7
Remove Item from method name since Cloud are not Items
Vlatombe Mar 30, 2023
b7acbcf
Implement StaplerProxy permission check trick
Vlatombe Mar 30, 2023
c6b3f4a
Use Functions#getAncestorUrl to compute current url
Vlatombe Mar 30, 2023
8dce53c
Remove Jenkins#getCloud()
Vlatombe Mar 30, 2023
8d754f6
Remove unused import
Vlatombe Mar 30, 2023
b41373b
Revert the previous change as we have a problem with views under Clou…
Vlatombe Mar 30, 2023
282a500
Permission check for descriptor as well through StaplerProxy
Vlatombe Mar 30, 2023
b4bc5cd
Seeing clouds only requires SYSTEM_READ
Vlatombe Mar 30, 2023
d67ad70
Remove unused import
Vlatombe Mar 31, 2023
1f266a0
l18n
Vlatombe Apr 4, 2023
94d4a9c
Update core/src/main/resources/jenkins/agents/CloudSet/index.jelly
Vlatombe Apr 4, 2023
305443a
No reason to expose getIcon to Rest API
Vlatombe Apr 5, 2023
c8ea200
Fix reviews
Vlatombe Apr 6, 2023
3c05c11
There is a shorter alternative
Vlatombe Apr 6, 2023
a7e46e0
Avoid descriptor.newInstance because of implementations doing field v…
Vlatombe Apr 7, 2023
95f42bd
Provide a default inline help for the name attribute of Cloud
Vlatombe Apr 7, 2023
de8ddc8
Ensure we don't end up with a Cloud without a name
Vlatombe Apr 7, 2023
3304d8f
Merge branch 'master' into manage-clouds
Vlatombe Apr 26, 2023
3aa80fb
When hitting a cloud with duplicate name, allow browsing it by index.
Vlatombe Apr 26, 2023
fcb6cd4
Avoid the transient field and compute on the fly
Vlatombe Apr 26, 2023
a5b312e
Add missing license header
Vlatombe Apr 26, 2023
ac990a9
Update core/src/main/java/jenkins/agents/CloudSet.java
timja Apr 26, 2023
45e656c
Make the config.jelly optional
Vlatombe Apr 27, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 67 additions & 1 deletion core/src/main/java/hudson/slaves/Cloud.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,23 @@
import hudson.security.PermissionScope;
import hudson.slaves.NodeProvisioner.PlannedNode;
import hudson.util.DescriptorList;
import java.io.IOException;
import java.util.Collection;
import java.util.Objects;
import java.util.concurrent.Future;
import javax.servlet.ServletException;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.DoNotUse;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.HttpRedirect;
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;
import org.kohsuke.stapler.export.Exported;
import org.kohsuke.stapler.interceptor.RequirePOST;
import org.kohsuke.stapler.verb.POST;

/**
* Creates {@link Node}s to dynamically expand/shrink the agents attached to Hudson.
Expand Down Expand Up @@ -122,7 +132,7 @@ public String getDisplayName() {
* @return Jenkins relative URL.
*/
public @NonNull String getUrl() {
return "cloud/" + Util.rawEncode(name);
return "cloud/" + Util.rawEncode(name) + "/";
}

@Override
Expand Down Expand Up @@ -275,6 +285,62 @@ public static void registerPermissions() {
Objects.hash(PERMISSION_SCOPE, PROVISION);
}

@Exported
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
public String getIcon() {
return "symbol-cloud";
}

public String getIconClassName() {
return "symbol-cloud";
}

@SuppressWarnings("unused") // stapler
public String getIconAltText() {
return getClass().getSimpleName().replace("Cloud", "");
}

/**
* Deletes the cloud.
*/
@RequirePOST
public HttpResponse doDoDelete() throws IOException {
checkPermission(Jenkins.ADMINISTER);
Jenkins.get().clouds.remove(this);
return new HttpRedirect("../..");
}

/**
* Accepts the update to the node configuration.
*/
@POST
public void doConfigSubmit(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException, Descriptor.FormException {
checkPermission(Jenkins.ADMINISTER);

String proposedName = Util.fixEmptyAndTrim(req.getSubmittedForm().getString("name"));
Jenkins.checkGoodName(proposedName);

Cloud cloud = Jenkins.get().getCloud(name);
if (cloud == null) {
throw new ServletException("No such cloud " + name);
}

if (!proposedName.equals(name)
&& Jenkins.get().getCloud(proposedName) != null) {
throw new Descriptor.FormException(jenkins.agents.Messages.CloudSet_CloudAlreadyExists(proposedName), "name");
}

Cloud result = cloud.reconfigure(req, req.getSubmittedForm());
Jenkins.get().clouds.replace(this, result);

// take the user back to the cloud top page.
rsp.sendRedirect2("../../" + result.getUrl());
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
}

public Cloud reconfigure(@NonNull final StaplerRequest req, JSONObject form) throws Descriptor.FormException {
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
if (form == null) return null;
return getDescriptor().newInstance(req, form);
}

/**
* Parameter object for {@link hudson.slaves.Cloud}.
* @since 2.259
Expand Down
209 changes: 209 additions & 0 deletions core/src/main/java/jenkins/agents/CloudSet.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@
package jenkins.agents;
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved

import hudson.Extension;
import hudson.Util;
import hudson.model.AbstractModelObject;
import hudson.model.AutoCompletionCandidates;
import hudson.model.Describable;
import hudson.model.Descriptor;
import hudson.model.Failure;
import hudson.model.RootAction;
import hudson.slaves.Cloud;
import hudson.util.FormValidation;
import java.io.IOException;
import java.util.List;
import java.util.stream.Collectors;
import javax.servlet.ServletException;
import jenkins.model.Jenkins;
import jenkins.model.ModelObjectWithChildren;
import jenkins.model.ModelObjectWithContextMenu;
import net.sf.json.JSONObject;
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.StaplerFallback;
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;
import org.kohsuke.stapler.export.Exported;
import org.kohsuke.stapler.export.ExportedBean;
import org.kohsuke.stapler.interceptor.RequirePOST;
import org.kohsuke.stapler.verb.POST;

@ExportedBean
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
@Extension
public class CloudSet extends AbstractModelObject implements Describable<CloudSet>, StaplerFallback, ModelObjectWithChildren, RootAction {
@Override
public Descriptor<CloudSet> getDescriptor() {
return Jenkins.get().getDescriptorOrDie(CloudSet.class);
}

@Override
public String getIconFileName() {
return null;
}

@Override
public String getDisplayName() {
return Messages.CloudSet_DisplayName();
}

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

@Override
public String getSearchUrl() {
return "/cloudSet/";
}

@Exported(name = "cloud", inline = true)
public Jenkins.CloudList get_all() {
return Jenkins.get().clouds;
}

@Override
public ModelObjectWithContextMenu.ContextMenu doChildrenContextMenu(StaplerRequest request, StaplerResponse response) throws Exception {
ModelObjectWithContextMenu.ContextMenu m = new ModelObjectWithContextMenu.ContextMenu();
get_all().stream().forEach(c -> m.add(c));
return m;
}

@Override
public Object getStaplerFallback() {
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
return Jenkins.get();
}

/**
* Gets all the agent names.
*/
@SuppressWarnings("unused") // stapler
public List<String> get_cloudNames() {
return Jenkins.get().clouds
.stream()
.map(c -> c.name)
.collect(Collectors.toList());
}

/**
* Makes sure that the given name is good as an agent name.
* @return trimmed name if valid; throws ParseException if not
*/
public String checkName(String name) throws Failure {
if (name == null)
throw new Failure("Query parameter 'name' is required");

name = name.trim();
Jenkins.checkGoodName(name);

if (Jenkins.get().getCloud(name) != null)
throw new Failure(Messages.CloudSet_CloudAlreadyExists(name));

// looks good
return name;
}

@SuppressWarnings("unused") // stapler
public FormValidation doCheckName(@QueryParameter String value) {
Jenkins.get().checkPermission(Jenkins.ADMINISTER);
if (Util.fixEmpty(value) == null) {
return FormValidation.ok();
}
try {
checkName(value);
return FormValidation.ok();
} catch (Failure e) {
return FormValidation.error(e.getMessage());
}
}

/**
* First check point in creating a new cloud.
*/
@RequirePOST
public synchronized void doCreateItem(StaplerRequest req, StaplerResponse rsp,
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
@QueryParameter String name, @QueryParameter String mode,
@QueryParameter String from) throws IOException, ServletException, Descriptor.FormException {
final Jenkins jenkins = Jenkins.get();
jenkins.checkPermission(Jenkins.ADMINISTER);

if (mode != null && mode.equals("copy")) {
name = checkName(name);

Cloud src = jenkins.getCloud(from);
if (src == null) {
if (Util.fixEmpty(from) == null) {
throw new Failure(Messages.CloudSet_SpecifyCloudToCopy());
} else {
throw new Failure(Messages.CloudSet_NoSuchCloud(from));
}
}

// copy through XStream
String xml = Jenkins.XSTREAM.toXML(src);
// Not great, but cloud name is final
xml = xml.replace("<name>" + src.name + "</name>", "<name>" + name + "</name>");
Cloud result = (Cloud) Jenkins.XSTREAM.fromXML(xml);
jenkins.clouds.add(result);
// send the browser to the config page
rsp.sendRedirect2(result.getUrl() + "configure");
} else {
// proceed to step 2
if (mode == null) {
throw new Failure("No mode given");
Copy link
Member

Choose a reason for hiding this comment

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

Not a user-friendly error message. Should be localizable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same issue in ItemGroupMixIn and ComputerSet. Should be addressed globally.

}

Descriptor<Cloud> d = Cloud.all().findByName(mode);
if (d == null) {
throw new Failure("No node type ‘" + mode + "’ is known");
Copy link
Member

@daniel-beck daniel-beck Mar 29, 2023

Choose a reason for hiding this comment

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

Refers to nodes when we're configuring clouds.

Should be localizable.

}
handleNewCloudPage(d, name, req, rsp);
}
}

private void handleNewCloudPage(Descriptor<Cloud> descriptor, String name, StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException, Descriptor.FormException {
checkName(name);
JSONObject formData = req.getSubmittedForm();
formData.put("name", name);
formData.put("cloudName", name);
formData.remove("mode"); // Cloud descriptors won't have this field.
Cloud instance = descriptor.newInstance(req, formData); // Not great but that's the best I have so far to pass the given name.
req.setAttribute("instance", instance);
req.setAttribute("descriptor", descriptor);
req.getView(this, "_new.jelly").forward(req, rsp);
}

/**
* Really creates a new agent.
*/
@POST
public synchronized void doDoCreateItem(StaplerRequest req, StaplerResponse rsp,
Copy link
Member

Choose a reason for hiding this comment

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

Weird naming scheme with …Item, given that clouds aren't Items.

@QueryParameter("_.name") String name,
@QueryParameter String type) throws IOException, ServletException, Descriptor.FormException {
final Jenkins app = Jenkins.get();
app.checkPermission(Jenkins.ADMINISTER);
String fixedName = Util.fixEmptyAndTrim(name);
checkName(fixedName);

JSONObject formData = req.getSubmittedForm();
formData.put("_.name", fixedName);

Cloud result = Cloud.all().find(type).newInstance(req, formData);
app.clouds.add(result);

// take the user back to the cloud list top page
rsp.sendRedirect2(".");
}
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved

@Extension
public static class DescriptorImpl extends Descriptor<CloudSet> {
/**
* Auto-completion for the "copy from" field in the new cloud page.
*/
public AutoCompletionCandidates doAutoCompleteCopyNewItemFrom(@QueryParameter final String value) {
daniel-beck marked this conversation as resolved.
Show resolved Hide resolved
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
final AutoCompletionCandidates r = new AutoCompletionCandidates();
Jenkins.get().clouds.stream()
.filter(c -> c.name.startsWith(value))
.forEach(c -> r.add(c.name));
return r;
}
}
}
37 changes: 37 additions & 0 deletions core/src/main/java/jenkins/agents/CloudsLink.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package jenkins.agents;

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import hudson.model.ManagementLink;
import org.jenkinsci.Symbol;

@Extension
@Symbol("clouds")
public class CloudsLink extends ManagementLink {

@Override
public String getDisplayName() {
return Messages.CloudsLink_DisplayName();
}

@Override
public String getDescription() {
return Messages.CloudsLink_Description();
}

@Override
public String getIconFileName() {
return "symbol-cloud";
}

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

@NonNull
@Override
public Category getCategory() {
return Category.CONFIGURATION;
}
}
2 changes: 1 addition & 1 deletion core/src/main/java/jenkins/management/NodesLink.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public class NodesLink extends ManagementLink {

@Override
public String getIconFileName() {
return "symbol-cloud";
return "symbol-computer";
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import hudson.model.Job;
import hudson.model.ModelObject;
import hudson.model.Node;
import hudson.slaves.Cloud;
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
Expand Down Expand Up @@ -207,6 +208,13 @@ public ContextMenu add(Computer c) {
.withContextRelativeUrl(c.getUrl()));
}

public ContextMenu add(Cloud c) {
return add(new MenuItem()
.withDisplayName(c.getDisplayName())
.withIconClass(c.getIconClassName())
.withContextRelativeUrl(c.getUrl()));
}

/**
* Adds a child item when rendering context menu of its parent.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ THE SOFTWARE.
<st:include page="sidepanel.jelly" />
<!-- no need for additional breadcrumb here as we're on an index page already including breadcrumb -->
<l:main-panel>
<l:app-bar title="${%Manage nodes and clouds}">
<l:app-bar title="${%Manage nodes}">
<l:hasAdministerOrManage>
<j:getStatic var="createPermission" className="hudson.model.Computer" field="CREATE"/>
<j:if test="${h.hasPermission(createPermission)}">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ THE SOFTWARE.
<l:header />
<l:side-panel>
<l:tasks>
<l:task href="../configureClouds" icon="symbol-cloud" permission="${app.SYSTEM_READ}"
title="${app.hasPermission(app.ADMINISTER) ? '%Configure Clouds' : '%View Clouds'}"/>
<l:task href="../computer" icon="symbol-computer" permission="${app.SYSTEM_READ}"
timja marked this conversation as resolved.
Show resolved Hide resolved
title="${app.hasPermission(app.ADMINISTER) ? '%Configure Nodes' : '%View Nodes'}"/>
<l:task href="configure" icon="symbol-settings" permissions="${app.MANAGE_AND_SYSTEM_READ}" title="${%Node Monitoring}"/>
</l:tasks>
<t:queue items="${app.queue.items}" />
Expand Down
Loading