-
-
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 17 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,57 @@ | ||
/* | ||
* 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.stapler.HttpResponse; | ||
import org.kohsuke.stapler.QueryParameter; | ||
|
||
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 | ||
* @since TODO | ||
*/ | ||
boolean isNameEditable(); | ||
|
||
/** | ||
* Renames the object | ||
* | ||
* @since TODO | ||
*/ | ||
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 | ||
* @since TODO | ||
*/ | ||
@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 @@ | |
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 { | ||
|
@@ -323,10 +390,8 @@ | |
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"); | ||
if (!(result.name).equals(this.name)) { | ||
throw new Descriptor.FormException(jenkins.agents.Messages.Cloud_DoNotRename(), "name"); | ||
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. Another option was to just ignore any name changes, i.e. 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. Will this case get triggered by some existing cloud plugins until they override 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 guess that is the case described in #8310 (comment). Any idea how widespread that kind of problem will be? Should we prepare downstream PRs? 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. Yes, this exception will be thrown by existing cloud nodes from plugins that do not have the form updated. I'll take a look to see what plugins are affected. 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 an initial search of "extends Cloud " and "exnteds AbstractCloudImpl" , I see around 50+ plugins that need to be checked. 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. Ideally, I'd like to just throw a warning dialogue box and block the save/apply action until then name change is reverted in the form. I'm just not sure how to do that. 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. Hmm, yeah I'm not sure how to pop a warning dialog here without having to update For now I would be tempted to just ignore name changes in this context. It would be pretty frustrating to reconfigure various things and attempt to rename an object at the same time, hit save, get an exception, and then have to reconfigure things again. IDK though how common it is to rename and reconfigure things at the same time. If people normally do them separately then the current behavior at least gives you clear feedback on why the rename did not happen vs just silently ignoring the rename. 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. (or file PRs into the major cloud plugins) 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 tried without any success to get the submit button to be blocked if the name is changed. Looks like I would have to use the I think I'll go ahead and just block the name change without the exception. I don't think name changes happen that much as this bug has only been discovered now. I think the fact that the rename page exists should be enough of a clue. I'll also go ahead and try and file some downstream PRs based off of this incremental. |
||
} | ||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
# The MIT License | ||
# | ||
# Copyright (c) 2021 Takashi Harano | ||
# | ||
# 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=名前の変更 {0} | ||
NewName=新しい名前 | ||
Rename=名前の変更 |
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