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.

Sign up for GitHub

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 #8505

Merged
merged 5 commits into from
Nov 29, 2023
Merged

[JENKINS-71737] fix redirect when submitting cloud changes #8505

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
acb34c3
update cloud config redirect, remove unused cloudName variable
car-roll Sep 19, 2023
40f0c35
remove apply button
car-roll Nov 17, 2023
f79afd8
Merge branch 'master' into redirect2
car-roll Nov 17, 2023
3446e00
Merge branch 'master' into redirect2
car-roll Nov 27, 2023
0f5de07
Merge branch 'master' into redirect2
NotMyFault Nov 28, 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
3 changes: 2 additions & 1 deletion core/src/main/java/hudson/slaves/Cloud.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,8 @@ public HttpResponse doConfigSubmit(StaplerRequest req, StaplerResponse rsp) thro
j.clouds.replace(this, result);
j.save();
// take the user back to the cloud top page.
return FormApply.success(".");
return FormApply.success("../" + result.name + '/');
Copy link
Contributor

@scherler scherler Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

for what it's worth, I think return FormApply.success("../.."); is the quick fix for the issue. Due to the underlying url manage/cloud/xyz/configure. You will always be in .../configure so you are forced to get back to the overall cloud listing page. If you want some elegance you test whether the name has not changed, then use the old way. However, since index.jelly is e.g. in kubernetes plugin a blank page and doing "." leads to it I would rather be redirected to the overall cloud page, but that is me as a user.

Copy link
Contributor Author

@car-roll car-roll Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@scherler .. was the original fix, but I think the main issue with that was that a quick fix was not desired, but rather a proper way to address the UX behavior
See #8310 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

a bug is a bug and should be fixed. Any follow-up should be addressed after that. That is just my 2 cents being a fan of iterations. The follow-up cost for any downstream plugin/code is to fix it themselves while we boil the ocean, which does not make sense at all. I know there will be a lot of "the follow-up never will come" excuses to not fix a BUG but that is a completely different discussion IMHO if the fix is ../.. and the behavior before the breaking change is back, there should not be a discussion. Really questionable why the fix has not landed 24 h after the PR had been filed. 🤷‍♂️


}

public Cloud reconfigure(@NonNull final StaplerRequest req, JSONObject form) throws Descriptor.FormException {
Expand Down
1 change: 0 additions & 1 deletion core/src/main/java/jenkins/agents/CloudSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,6 @@ private void handleNewCloudPage(Descriptor<Cloud> descriptor, String name, Stapl
checkName(name);
JSONObject formData = req.getSubmittedForm();
formData.put("name", name);
formData.put("cloudName", name); // ec2 uses that field name
Copy link
Contributor Author

@car-roll car-roll Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ec2 no longer uses that filed name
jenkinsci/ec2-plugin#892
also azure-vm-agents
jenkinsci/azure-vm-agents-plugin#462

formData.remove("mode"); // Cloud descriptors won't have this field.
req.setAttribute("instance", formData);
req.setAttribute("descriptor", descriptor);
Expand Down
1 change: 0 additions & 1 deletion core/src/main/resources/hudson/slaves/Cloud/configure.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ THE SOFTWARE.
<l:isAdmin>
<f:bottomButtonBar>
<f:submit value="${%Save}"/>
<f:apply />
</f:bottomButtonBar>
</l:isAdmin>
</f:form>
Expand Down