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

car-roll · 2023-09-19T04:39:10Z

#8310 first started as simple redirect fixes, but they never worked for downstream plugins (particularly ec2 and azure-vm-agents). The solution then evolved to make a separate rename page that was consistent with the rename logic of AbstractItem introduced in #3289 .

Eventually it was discovered that the use of cloudName instead of name in the downstream plugins, along with a hardcoded workaround in core was what made my initial attempts at redirect fail.

This PR is an alternative to the more complicated #8310
Fixes the redirect so that save goes to the new cloud name page and it removes the apply button from cloud configuration

Testing done

Tested manually in ec2 and azure-vm-agents
TODO: Create PRs with incremental generated by this PR

Proposed changelog entries

Fix redirect when renaming a cloud

Proposed upgrade guidelines

N/A

Submitter checklist

Give feedback

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.
Options

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Give feedback

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
Options

timja

doesn't work if someone clicks apply and then makes another change.

(the main reason for the rename page I thought was to avoid the apply button being present)

car-roll · 2023-09-19T14:25:04Z

@timja you are right, in my excitement I forgot about that part 🤦

timja · 2023-09-24T15:48:00Z

@timja you are right, in my excitement I forgot about that part 🤦

Basil suggested on the other PR you could just remove the apply button and that may be the simplest fix and then going with this

scherler · 2023-10-20T11:22:09Z

core/src/main/java/hudson/slaves/Cloud.java

@@ -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 + '/');


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.

@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)

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. 🤷‍♂️

scherler · 2023-10-20T12:06:28Z

https://github.com/scherler/jenkins/tree/JENKINS-71737 is the test that should pass

Vlatombe · 2023-11-15T13:16:07Z

Basil suggested on the other PR you could just remove the apply button and that may be the simplest fix and then going with this

Can we go with this and forget about this bug?

timja · 2023-11-15T13:55:19Z

Basil suggested on the other PR you could just remove the apply button and that may be the simplest fix and then going with this

Can we go with this and forget about this bug?

Why not just remove the apply button?

Vlatombe · 2023-11-15T15:38:21Z

Yes, this is what I meant.

timja · 2023-11-15T20:29:50Z

Yes, this is what I meant.

go for it

car-roll · 2023-11-17T19:20:01Z

core/src/main/java/jenkins/agents/CloudSet.java

@@ -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


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

timja

LGTM

timja · 2023-11-28T09:45:28Z

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

…#8505) Co-authored-by: Alexander Brandes <[email protected]> (cherry picked from commit ec27a07)

update cloud config redirect, remove unused cloudName variable

acb34c3

car-roll mentioned this pull request Sep 19, 2023

[JENKINS-71737] Fix redirect when submitting cloud changes #8310

Closed

5 tasks

timja reviewed Sep 19, 2023

View reviewed changes

scherler reviewed Oct 20, 2023

View reviewed changes

car-roll added 2 commits November 17, 2023 12:10

remove apply button

40f0c35

Merge branch 'master' into redirect2

f79afd8

car-roll commented Nov 17, 2023

View reviewed changes

timja approved these changes Nov 17, 2023

View reviewed changes

Merge branch 'master' into redirect2

3446e00

timja requested review from scherler and a team November 28, 2023 09:02

NotMyFault approved these changes Nov 28, 2023

View reviewed changes

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 28, 2023

car-roll changed the title ~~[JENKINS-71737] Simpler fix of redirect when submitting cloud changes~~ [JENKINS-71737] fix redirect when submitting cloud changes Nov 28, 2023

Merge branch 'master' into redirect2

0f5de07

timja merged commit ec27a07 into jenkinsci:master Nov 29, 2023
17 checks passed

krisstern pushed a commit to krisstern/jenkins that referenced this pull request Jan 8, 2024

[JENKINS-71737] fix redirect when submitting cloud changes (jenkinsci…

8908239

…#8505) Co-authored-by: Alexander Brandes <[email protected]> (cherry picked from commit ec27a07)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

car-roll commented Sep 19, 2023 •

edited

Loading

Submitter checklist

Maintainer checklist

timja left a comment

car-roll commented Sep 19, 2023

timja commented Sep 24, 2023

scherler Oct 20, 2023 •

edited

Loading

car-roll Oct 20, 2023 •

edited

Loading

scherler Oct 23, 2023

scherler commented Oct 20, 2023

Vlatombe commented Nov 15, 2023

timja commented Nov 15, 2023

Vlatombe commented Nov 15, 2023

timja commented Nov 15, 2023

car-roll Nov 17, 2023 •

edited

Loading

timja left a comment

timja commented Nov 28, 2023

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

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

Conversation

car-roll commented Sep 19, 2023 • edited Loading

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

timja left a comment

Choose a reason for hiding this comment

car-roll commented Sep 19, 2023

timja commented Sep 24, 2023

scherler Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

car-roll Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

scherler Oct 23, 2023

Choose a reason for hiding this comment

scherler commented Oct 20, 2023

Vlatombe commented Nov 15, 2023

timja commented Nov 15, 2023

Vlatombe commented Nov 15, 2023

timja commented Nov 15, 2023

car-roll Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

timja left a comment

Choose a reason for hiding this comment

timja commented Nov 28, 2023

car-roll commented Sep 19, 2023 •

edited

Loading

scherler Oct 20, 2023 •

edited

Loading

car-roll Oct 20, 2023 •

edited

Loading

car-roll Nov 17, 2023 •

edited

Loading