-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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-71737] Fix redirect when submitting cloud changes #8310
Changes from 29 commits
081cadd
cb2a5fd
1741953
a0a3e9c
3d50865
29ad035
ff3cbba
dbf61fd
108bc22
cc7c4e1
e3ef831
7a96cd6
b8e7f84
aec2de1
d5b6dcd
a57343c
73f451e
b7eed6f
6c561f7
10ea3dd
92489f1
b6e2fde
5bb029f
63c3633
bf62fc2
1024b9e
4b89c49
8c3d6c6
231db94
13bab6c
e933c51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
/* | ||
* The MIT License | ||
* | ||
* Copyright 2023 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 hudson.model; | ||
|
||
import edu.umd.cs.findbugs.annotations.NonNull; | ||
import hudson.util.FormValidation; | ||
import org.kohsuke.accmod.Restricted; | ||
import org.kohsuke.accmod.restrictions.ProtectedExternally; | ||
import org.kohsuke.stapler.HttpResponse; | ||
import org.kohsuke.stapler.QueryParameter; | ||
|
||
/** | ||
* Interface used to create a dedicated page that changes the name property of an Object. | ||
* See also {@link jenkins.model.RenameAction}. | ||
* | ||
* @since TODO | ||
*/ | ||
@Restricted(ProtectedExternally.class) | ||
public interface Renamable { | ||
dwnusbaum marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having a hard time understanding the relationship between this interface and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea was to make them implement this interface as well: #8310 (comment) I had originally tried to do it the same way as Computer, but it always failed testing it in other plugins such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @timja reminded me that one of the big issues was that apply does not work correctly when changing names and other config parts: #8505 (review) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To fully answer the original comment, I guess it is more a matter of being consistent across our UIs? Changing the name of a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the flip side, why not remove the Apply button from A request to defer implementing this interface in When defining a new interface, it is important to consume it in at least three places before shipping it to ensure the design is correct. If you implement it only once, it probably won't support a second implementation. If you implement it only twice, it will probably support a third implementation with difficulty. If you implement it three or more times, the interface will probably work fine. Will Tracz calls this "The Rule of Threes" (Confessions of a Used Program Salesman, Addison-Wesley, 1995). So my perspective is that the third implementation should be in scope for this PR not only in order to make the implementation complete but also to validate the design of the interface. |
||
|
||
/** | ||
* Controls whether the default rename action is available for this object. | ||
* | ||
* @return whether the name can be modified by a user | ||
*/ | ||
boolean isNameEditable(); | ||
|
||
/** | ||
* Renames the object | ||
* | ||
*/ | ||
HttpResponse doConfirmRename(@QueryParameter String newName) throws Exception; | ||
|
||
/** | ||
* Controls whether the default rename action is available. | ||
* | ||
* @return whether object name can be modified by a user | ||
*/ | ||
@NonNull | ||
FormValidation doCheckNewName(@QueryParameter String newName); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,15 +30,18 @@ | |
import hudson.DescriptorExtensionList; | ||
import hudson.Extension; | ||
import hudson.ExtensionPoint; | ||
import hudson.Functions; | ||
import hudson.Util; | ||
import hudson.init.InitMilestone; | ||
import hudson.init.Initializer; | ||
import hudson.model.Actionable; | ||
import hudson.model.Computer; | ||
import hudson.model.Describable; | ||
import hudson.model.Descriptor; | ||
import hudson.model.Failure; | ||
import hudson.model.Label; | ||
import hudson.model.Node; | ||
import hudson.model.Renamable; | ||
import hudson.model.Slave; | ||
import hudson.security.ACL; | ||
import hudson.security.AccessControlled; | ||
|
@@ -47,6 +50,7 @@ | |
import hudson.slaves.NodeProvisioner.PlannedNode; | ||
import hudson.util.DescriptorList; | ||
import hudson.util.FormApply; | ||
import hudson.util.FormValidation; | ||
import java.io.IOException; | ||
import java.util.Collection; | ||
import java.util.Objects; | ||
|
@@ -57,9 +61,12 @@ | |
import org.apache.commons.lang.Validate; | ||
import org.kohsuke.accmod.Restricted; | ||
import org.kohsuke.accmod.restrictions.DoNotUse; | ||
import org.kohsuke.accmod.restrictions.NoExternalUse; | ||
import org.kohsuke.stapler.DataBoundConstructor; | ||
import org.kohsuke.stapler.HttpRedirect; | ||
import org.kohsuke.stapler.HttpResponse; | ||
import org.kohsuke.stapler.HttpResponses; | ||
import org.kohsuke.stapler.QueryParameter; | ||
import org.kohsuke.stapler.StaplerRequest; | ||
import org.kohsuke.stapler.StaplerResponse; | ||
import org.kohsuke.stapler.interceptor.RequirePOST; | ||
|
@@ -107,7 +114,7 @@ | |
* @see NodeProvisioner | ||
* @see AbstractCloudImpl | ||
*/ | ||
public abstract class Cloud extends Actionable implements ExtensionPoint, Describable<Cloud>, AccessControlled { | ||
public abstract class Cloud extends Actionable implements ExtensionPoint, Describable<Cloud>, AccessControlled, Renamable { | ||
|
||
/** | ||
* Uniquely identifies this {@link Cloud} instance among other instances in {@link jenkins.model.Jenkins#clouds}. | ||
|
@@ -310,8 +317,68 @@ public HttpResponse doDoDelete() throws IOException { | |
return new HttpRedirect(".."); | ||
} | ||
|
||
|
||
@Override | ||
public boolean isNameEditable() { | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I must be missing something, because I can't see how renaming is exposed in the GUI for, say, the Docker plugin. I have a Docker cloud created against a core before this PR and the current release of Docker plugin, and I have another Docker cloud created against a core after this PR and jenkinsci/docker-plugin#1016. I can visit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rename page gets exposed via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still couldn't find where |
||
} | ||
|
||
@RequirePOST | ||
@Restricted(NoExternalUse.class) | ||
@Override | ||
public HttpResponse doConfirmRename(@QueryParameter String newName) throws IOException, ServletException, Descriptor.FormException { | ||
newName = newName == null ? null : newName.trim(); | ||
FormValidation validationError = doCheckNewName(newName); | ||
if (validationError.kind != FormValidation.Kind.OK) { | ||
throw new Failure(validationError.getMessage()); | ||
} | ||
this.name = newName; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we be doing something to persist this change to disk, the same way it would be persisted immediately if we changed it via the configure page? |
||
|
||
// take the user to the renamed cloud top page. | ||
return HttpResponses.redirectTo("../" + Functions.encode(newName)); | ||
} | ||
|
||
@NonNull | ||
@Restricted(NoExternalUse.class) | ||
@Override | ||
public FormValidation doCheckNewName(String newName) { | ||
if (!isNameEditable()) { | ||
return FormValidation.error("Trying to rename an item that does not support this operation."); | ||
} | ||
checkPermission(Jenkins.ADMINISTER); | ||
|
||
newName = newName == null ? null : newName.trim(); | ||
|
||
try { | ||
Jenkins.checkGoodName(newName); | ||
assert newName != null; // Would have thrown Failure | ||
if (newName.equals(name)) { | ||
return FormValidation.warning(hudson.model.Messages.AbstractItem_NewNameUnchanged()); | ||
} | ||
if (Jenkins.get().getCloud(newName) != null) { | ||
return FormValidation.warning(jenkins.agents.Messages.CloudSet_CloudAlreadyExists(newName)); | ||
} | ||
checkRename(newName); | ||
} catch (Failure e) { | ||
return FormValidation.error(e.getMessage()); | ||
} | ||
return FormValidation.ok(); | ||
} | ||
|
||
/** | ||
* Allows subclasses to block renames for domain-specific reasons. Generic validation of the new name | ||
* (e.g., null checking, checking for illegal characters, and checking that the name is not in use) | ||
* always happens prior to calling this method. | ||
* | ||
* @param newName the new name for the item | ||
* @throws Failure if the rename should be blocked | ||
*/ | ||
protected void checkRename(@NonNull String newName) throws Failure { | ||
|
||
} | ||
|
||
/** | ||
* Accepts the update to the node configuration. | ||
* Accepts the update to the node configuration. To change node name see {@link #doConfirmRename(String)}. | ||
*/ | ||
@POST | ||
public HttpResponse doConfigSubmit(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException, Descriptor.FormException { | ||
|
@@ -322,11 +389,13 @@ public HttpResponse doConfigSubmit(StaplerRequest req, StaplerResponse rsp) thro | |
if (cloud == null) { | ||
throw new ServletException("No such cloud " + this.name); | ||
} | ||
Cloud result = cloud.reconfigure(req, req.getSubmittedForm()); | ||
String proposedName = result.name; | ||
timja marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!proposedName.equals(this.name) | ||
&& j.getCloud(proposedName) != null) { | ||
throw new Descriptor.FormException(jenkins.agents.Messages.CloudSet_CloudAlreadyExists(proposedName), "name"); | ||
JSONObject formData = req.getSubmittedForm(); | ||
// add the cloud name to the submitted form data | ||
formData.put("name", name); | ||
Cloud result = cloud.reconfigure(req, formData); | ||
if (!(result.name).equals(this.name)) { | ||
// Do not rename the cloud in the config page. Use doConfirmRename() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Who is the intended audience for this imperative statement? If this is temporary logic to support some sort of migration period, then a warning should be logged that the cloud is using an unsupported mechanism that will be eventually removed in the future. (And if so, that also raises the question, why support this but not creating a new cloud with a plugin that still uses the configure page during the same migration period?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, originally I had it as a form exception and I just wanted to block old/incorrect implementations in the downstream plugins' Another attempt I tried to disable the save button if the name was changed, but it looked like i couldn't do that with the submit button, and I would have to do something with the regular button. At that point I stopped since I did not think it was worth the risk doing changes I wasn't very sure of doing in the first place. |
||
result.name = this.name; | ||
} | ||
j.clouds.replace(this, result); | ||
j.save(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
<!-- | ||
The MIT License | ||
|
||
Copyright 2018 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. | ||
--> | ||
|
||
<?jelly escape-by-default='true'?> | ||
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt"> | ||
<l:layout title="${%Rename}"> | ||
<st:include page="sidepanel.jelly" /> | ||
<l:breadcrumb title="${%Rename}" /> | ||
<l:main-panel> | ||
<h1>${%DescribeRename(it.name)}</h1> | ||
<f:form method="post" action="confirmRename" name="config" tableClass="config-table scrollspy"> | ||
<f:entry title="${%NewName}"> | ||
<f:textbox name="newName" value="${it.name}" autocomplete="on" checkUrl="checkNewName" checkDependsOn="newName"/> | ||
</f:entry> | ||
<f:bottomButtonBar> | ||
<f:submit value="${%Rename}" /> | ||
</f:bottomButtonBar> | ||
</f:form> | ||
<st:adjunct includes="lib.form.confirm" /> | ||
</l:main-panel> | ||
</l:layout> | ||
</j:jelly> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
# The MIT License | ||
# | ||
# Copyright (c) 2018 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. | ||
|
||
DescribeRename=Rename {0} | ||
NewName=New Name | ||
Rename=Rename |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
DescribeRename={0} umbenennen | ||
NewName=Neuer Name | ||
Rename=Umbenennen |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Rename=Renommer |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# The MIT License | ||
# | ||
# Italian localization plugin for Jenkins | ||
# Copyright © 2020 Alessandro Menti | ||
# | ||
# 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. | ||
|
||
DescribeRename=Rinomina {0} | ||
NewName=Nuovo nome | ||
Rename=Rinomina |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that these restrictions become easily bypassable via casting to
Renamable
with the new changes. Perhaps the new interface should be marked as@Restricted(ProtectedExternally.class)
? Even then there would be some issues though, see jenkinsci/lib-access-modifier#22 and jenkinsci/lib-access-modifier#21.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
@Restricted(ProtectedExternally.class)
in b7eed6f