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

Move cloud configuration out of Configure System #4339

Merged
merged 3 commits into from
Nov 16, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
54 changes: 40 additions & 14 deletions core/src/main/java/jenkins/model/GlobalCloudConfiguration.java
Original file line number Diff line number Diff line change
@@ -1,30 +1,56 @@
package jenkins.model;

import hudson.Extension;
import hudson.RestrictedSince;
import hudson.model.Descriptor;
import hudson.model.RootAction;
import hudson.slaves.Cloud;
import hudson.util.FormApply;
import net.sf.json.JSONObject;
import org.jenkinsci.Symbol;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;
import org.kohsuke.stapler.verb.POST;

import javax.annotation.CheckForNull;
import javax.servlet.ServletException;
import java.io.IOException;

/**
* Adds the {@link Cloud} configuration to the system config page.
* Provides a configuration form for {@link Jenkins#clouds}.
*
* <p>
* This object just acts as a proxy to configure {@link Jenkins#clouds}
*
* @author Kohsuke Kawaguchi
* Has been overhauled in Jenkins 2.XXX to no longer contribute to Configure System, but be a standalone form.
*/
@Extension(ordinal=-100) @Symbol("cloud") // historically this was placed at the very end of the configuration page
public class GlobalCloudConfiguration extends GlobalConfiguration {
@Extension
Copy link

Choose a reason for hiding this comment

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

Why the ordinal before?

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment on the same line explains it.

@Symbol("cloud")
@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.

This would prohibit JCasC from configuring it by default

Copy link
Member

@timja timja Nov 14, 2019

Choose a reason for hiding this comment

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

JCasC ignores it on the class level, only looks at it on getters and constructors afaik

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@timja wrote on Sunday:

Tested manually with JCasC + the azure-vm agents plugin, all fine

So please provide some supporting evidence for your claim.

Copy link
Member

Choose a reason for hiding this comment

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

OK. I'd guess it is rather a subject for the future when JCasC starts consulting with class- and package- level annotations for such use-cases. Does not block it now

@RestrictedSince("TODO")
public class GlobalCloudConfiguration implements RootAction {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change here seems to break the public API.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, given the lack of usages in plugins (both via GH search and jenkins-infra/usage-in-plugins, I decided to overhaul the class and restrict it in the process, instead of dancing around that.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this PR will impact JCasC but if anything out of this change does it will be this line. Needs testing

Copy link
Member

Choose a reason for hiding this comment

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

In the past when we moved for example the tool configurations out of the global config page it was done in a different way. IIRC it was still left as a GlobalConfiguration class but given a new category that got excluded from global config page and then included on a ManagementLink page.
Why not use a similar approach here? The old class could be left quite intact and you can add a separate RootAction if that shows the management page?

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 is slightly different since there's only one hetero-list on this page ever -- that of cloud descriptors. That's also why there's no new ConfigurationCategory.

So I chose to repurpose this class with some minor code changes, rather than write a new class that ends up just being a minimal wrapper view for the existing view fragment, and having the doConfigure endpoint that just delegates to the existing configure.


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

@CheckForNull
Copy link
Member

Choose a reason for hiding this comment

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

does this need check for null on it?

@Override
public boolean configure(StaplerRequest req, JSONObject json) throws FormException {
try {
Jenkins.get().clouds.rebuildHetero(req,json, Cloud.all(), "cloud");
return true;
} catch (IOException e) {
throw new FormException(e,"clouds");
}
public String getDisplayName() {
return Messages.GlobalCloudConfiguration_DisplayName();
}

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

@POST
public void doConfigure(StaplerRequest req, StaplerResponse rsp) throws Descriptor.FormException, IOException, ServletException {
Jenkins.get().checkPermission(Jenkins.ADMINISTER);
JSONObject json = req.getSubmittedForm();
Jenkins.get().clouds.rebuildHetero(req,json, Cloud.all(), "cloud");
FormApply.success(req.getContextPath() + "/manage").generateResponse(req, rsp, null);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ THE SOFTWARE.
<l:task href="${rootURL}/" icon="icon-up icon-md" title="${%Back to Dashboard}"/>
<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-gear2 icon-md" permission="${app.ADMINISTER}" title="${%Configure}"/>
<l:task href="${rootURL}/configureClouds" icon="icon-health-40to59 icon-md" permission="${app.ADMINISTER}" title="${%Configure Clouds}"/>
Copy link
Member

Choose a reason for hiding this comment

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

is this the only way (except the legacy one) to get to the configure clouds page now? In which case I would not expect Clouds to be managed via "Manage Nodes" and this would be confusing and not pass the path of least surprise.

I think @rsandell had a slightly similar concern.

Copy link
Member Author

@daniel-beck daniel-beck Nov 7, 2019

Choose a reason for hiding this comment

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

Why would you not expect to configure clouds ("node factories") on "manage nodes"?

I think @rsandell had a slightly similar concern.

I understood his question to be more about the repurposing of the existing class as a standalone page rather than add a wrapper class. I responded to that separately.

Copy link
Member

Choose a reason for hiding this comment

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

I don't expect to see tyre factories what I go to get new tyres from the tyre shop... (sorry I just can't find a better analogy right now).

We have told users that Nodes are the things that run their jobs - not the things that can create things that can run their jobs. Thus I think this is confusing. (it's not called "Manage Nodes and Node Factories" and thus not obvious) - I'm not sure what you have against a separate "Manage Clouds" section in the Management area)

Also if I am not mistaken Nodes (Computers) have different Permissions to Clouds (Clouds don't have a separate Permissions from Administer).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you have against a separate "Manage Clouds" section in the Management area

Seems like a redundant entry when a suitable entry page already exists.

it's not called "Manage Nodes and Node Factories" and thus not obvious

So, rename this to Manage Nodes and Clouds and done?

Copy link
Member

Choose a reason for hiding this comment

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

So, rename this to Manage Nodes and Clouds and done?

works for me

Copy link
Member Author

@daniel-beck daniel-beck Nov 8, 2019

Choose a reason for hiding this comment

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

why do we call it Nodes and not agents anyway?

Because you can also monitor master and configure master node properties, and master is not an agent.

Remember the term that 'agent' is the replacement for.

Copy link
Member

Choose a reason for hiding this comment

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

OT: I really want to kill the Master as a Node....

Copy link
Member

Choose a reason for hiding this comment

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

If a Node is something with Executors, then yeah, it'd be great to remove that functionality from the master since it's a poor setup.

Copy link
Member

Choose a reason for hiding this comment

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

If a Node is something with Executors, then yeah, it'd be great to remove that functionality from the master since it's a poor setup.

You can have a node with no executors. it will just run Flyweight tasks. However you can not create a Node like that via the UI (except for the master)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, it'd be great to remove that functionality from the master since it's a poor setup.

It's been 0 days since you've experienced how cumbersome it is to set up Jenkins with agents, and now you want to make that mandatory? 😁

FWIW, if properly analyzed, usage stats would show that by number of instances, 0 agents is the most popular configuration by far. I've seen that a few years ago when I had to manually wrestle the SQLite DB. IOW, we'd break a lot instances.

This is a nice goal, but there's actual work to be done here to even get closer.

<l:task href="configure" icon="icon-gear2 icon-md" permission="${app.ADMINISTER}" title="${%Node Monitoring}"/>
</l:tasks>
<t:queue items="${app.queue.items}" />
<t:executors />
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package jenkins.model.GlobalCloudConfiguration

import hudson.slaves.Cloud


def f = namespace(lib.FormTagLib)
def l = namespace(lib.LayoutTagLib)
def st = namespace("jelly:stapler")

l.layout(norefresh:true, permission:app.ADMINISTER, title:my.displayName) {
l.side_panel {
l.tasks {
l.task(icon:"icon-up icon-md", href:rootURL+'/', title:_("Back to Dashboard"))
l.task(icon:"icon-gear2 icon-md", href:"${rootURL}/computer/", title:_("Manage Nodes"))
}
}
l.main_panel {
h1 {
l.icon(class: 'icon-health-40to59 icon-xlg')
// TODO more appropriate icon
Copy link
Member

Choose a reason for hiding this comment

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

does this belong one line up? i.e. above the icon.
and is it still relevant?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. icon-health-40to59 looks like a hack to me. Personally I find it funny (it's a cloud), but wouldn't be surprised if others are confused by this icon.

(TBH, I just it copied from the tool configuration view, but it's oddly applicable here as well, so can be left in.)

Re the exact positioning of the TODO comment while the context is clear, I think it doesn't matter.

text(my.displayName)
}

def clouds = Cloud.all()

if (!clouds.isEmpty()) {
p()
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, what's the reason for putting an empty p tag before the other content?

Copy link
Member Author

Choose a reason for hiding this comment

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

div(class:"behavior-loading", _("LOADING"))

f.form(method:"post",name:"config",action:"configure") {
f.block {
f.hetero_list(name:"cloud", hasHeader:true, descriptors:Cloud.all(), items:app.clouds,
addCaption:_("Add a new cloud"), deleteCaption:_("Delete cloud"))
}

f.bottomButtonBar {
f.submit(value:_("Save"))
f.apply(value:_("Apply"))
}
}
st.adjunct(includes: "lib.form.confirm")
} else {
p {
_("There are no cloud implementations for dynamically allocated agents installed.")
a(href: rootURL + "/pluginManager/available") {
_("Go to plugin manager.")
}
}
}
}
}
17 changes: 17 additions & 0 deletions core/src/main/resources/jenkins/model/Jenkins/_cloud-note.groovy
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package jenkins.model.Jenkins

import hudson.slaves.Cloud

// TODO remove this once it's been long enough that users got used to this.

def f = namespace(lib.FormTagLib)

def clouds = Cloud.all()

if (!clouds.isEmpty()) {
f.block {
div(class: 'alert alert-info') {
raw(_("note", rootURL))
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
note=The cloud configuration has moved to <a href="{0}/configureClouds">a separate configuration page</a>.
2 changes: 2 additions & 0 deletions core/src/main/resources/jenkins/model/Jenkins/configure.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ THE SOFTWARE.
</f:rowSet>
</j:forEach>

<st:include page="_cloud-note.jelly"/>

<f:bottomButtonBar>
<f:submit value="${%Save}" />
<f:apply />
Expand Down
2 changes: 2 additions & 0 deletions core/src/main/resources/jenkins/model/Messages.properties
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,5 @@ BuildDiscarderProperty.displayName=Discard old builds
EnforceSlaveAgentPortAdministrativeMonitor.displayName=Enforce TCP Agent Port
CLI.disable-job.shortDescription=Disables a job.
CLI.enable-job.shortDescription=Enables a job.

GlobalCloudConfiguration.DisplayName=Configure Clouds