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

car-roll · 2023-07-28T18:27:18Z

Discovered in jenkinsci/ec2-plugin#875
Changing the cloud node name returns a 404. The cloud node name does get changed successfully, it is just the redirect is bad because it is trying to go to the old cloud name.

Testing done

Added test changing cloud name. Test fails with the existing implementation and now passes in this PR.
Downstream plugin testing:

docker-plugin: Require Jenkins 2.414.3 or newer docker-plugin#1016
kubernetes-plugin: [JENKINS-71737] refactor to support dedicated cloud rename page kubernetes-plugin#1432
ec2-plugin: [JENKINS-71737] update for dedicated cloud rename page ec2-plugin#891, [JENKINS-71737] rename cloudName to name ec2-plugin#892
azure-vm-agents-plugin: [JENKINS-71737] refactor to support dedicated cloud rename page azure-vm-agents-plugin#460, [JENKINS-71737] rename cloudName to name azure-vm-agents-plugin#462
mesos-plugin: [JENKINS-71737] make MesosCloud name uneditable mesos-plugin#420 (best effort, not familiar with gradle builds) - Last significant release was 3 years ago, so not sure if this is a priority here PR on hold as the gradle builds in this repo do not support jenkins incrementals

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

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

jglick

(amending #7658)

jtnord · 2023-07-28T22:36:23Z

Can you file a Jira ticker and mark as an LTS candidate?

basil · 2023-07-31T20:39:31Z

Can this not be tested with e.g. DummyCloudImpl and HtmlUnit?

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

car-roll · 2023-08-15T06:07:15Z

Converting to draft as there is more work to be done here.

car-roll · 2023-08-16T19:17:01Z

So I was in the process of implementing the dedicated name page, but I was wondering if just blocking the user from clicking "apply" would do the trick as well whenever there is a detected name change?

cc @mawinter69 @timja

if that doesn't work either, I'm fine with another dedicated page. I just thought it was nicer to keep everything on one page.

Vlatombe · 2023-08-17T14:30:32Z

I just thought it was nicer to keep everything on one page.

On the long run, having consistency is better than having a local optimum. This is probably an opportunity to generalize the current "rename" concept that currently only applies to AbstractItem, so that it becomes an interface you can apply to anything (AbstractItem, Cloud, Node, etc.) you wish to rename.

timja · 2023-08-17T14:44:21Z

So I was in the process of implementing the dedicated name page, but I was wondering if just blocking the user from clicking "apply" would do the trick as well whenever there is a detected name change?

Not great UX, they could have already done a bunch of changes, if apply isn't possible it shouldn't be there, i.e. imo better to be consistent and use a different page.

This reverts commit 108bc22.

car-roll · 2023-09-13T03:33:28Z

I have been able to manually confirm the changes are working for docker-plugin, kubernetes-plugin, ec2-plugin, and azure-vm-agents-plugin. mesos-plugin I am uncertain as I do not use gradle, and have been having difficulty getting a working environment to test my changes.

car-roll · 2023-09-13T18:46:09Z

After confirming the downstream plugin behaviors with the incremental, I believe this PR is ready for review again.

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

car-roll · 2023-09-15T18:36:34Z

now that jenkinsci/azure-vm-agents-plugin#462 and jenkinsci/ec2-plugin#892 have been merged, I updated all the incrementals of the downstream plugins and confirmed cloud creation, renaming, updating, and deletion are working as expected.

basil · 2023-09-15T21:36:23Z

core/src/main/java/hudson/model/Renamable.java

+ * @since TODO
+ */
+@Restricted(ProtectedExternally.class)
+public interface Renamable {


Having a hard time understanding the relationship between this interface and Node/Slave. Should they implement this interface or not? If so, why don't they? If not, why does renaming work without issue there?

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 ec2 and azure-vm-agents.
Looking back now, I realize the problem was always the cloudName vs name issue that was finally identified and corrected. I have just pushed up a second, simpler, version of a fix for this bug. See #8505

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

To fully answer the original comment, I guess it is more a matter of being consistent across our UIs? Changing the name of a Node isn't an issue because there is no Apply button. Should we implement this interface in Node, the apply button could be added back in.
Given how this PR affected a bunch of downstream PRs, it's probably best to implement this interface one component at a time.

On the flip side, why not remove the Apply button from Cloud to make it consistent with Node? That's inconsistent with AbstractItem, but so is the status quo where Node is inconsistent with AbstractItem. And at least it fixes the bug.

A request to defer implementing this interface in Node is effectively a request to introduce debt. I would not automatically reject such a request, but I think it needs to be justified, and merely mentioning "how this PR affected a bunch of downstream PRs" does not seem like a valid justification to me. And I think the intention behind the request is to implement in stages (which is fine, if the rest of the stages are actually finished in a timely fashion, something which almost never happens in my experience whenever someone makes such a request), but an unintentional consequence is that the design of the interface might turn out worse.

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.

basil · 2023-09-15T21:42:50Z

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

+
+    @Override
+    public boolean isNameEditable() {
+        return true;


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 confirm-rename manually and rename the cloud there, but I can't get the link to show up anywhere in the GUI for either of my two clouds. Please forgive me if I am doing something wrong on my side.

The rename page gets exposed via TransientActionFactoryImpl, which is hidden in RenameAction. I had to get pointed in the right direction as well when I was first looking into this.
I was able to create an old docker cloud and then when I changed to jenkinsci/docker-plugin#1016, was able to see the old docker cloud name and then rename it though. But to be fair, I didn't really have any parameters filled in my cloud.

I still couldn't find where RenameAction is visible in the UI on Cloud with these changes even after looking on several pages. I must be missing something.

basil · 2023-09-15T21:43:16Z

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

+        if (validationError.kind != FormValidation.Kind.OK) {
+            throw new Failure(validationError.getMessage());
+        }
+        this.name = newName;


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?

basil · 2023-09-15T21:44:49Z

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

+        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()


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

Well, originally I had it as a form exception and I just wanted to block old/incorrect implementations in the downstream plugins' config.jelly. My rationale for a quiet ignore was that there is a large sidebar link that says rename so if hopefully no one would get too frustrated before noticing that. I couldn't find a nice way to pop up a warning message for someone who clicked save. Apply, was possible, but I couldn't figure out when you clicked saved.

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.

car-roll · 2023-09-19T04:43:13Z

Now that cloudName and name have been sorted out, I have pushed up a much simpler fix for the redirect bug in #8505 . Thanks to @basil 's comment that made me revisit some of my earlier attempts.

If the simpler fix is accepted, I guess this PR might be considered a proposed UI enhancement? If there is a consensus to continue it, that is.

timja · 2023-09-19T07:57:08Z

Now that cloudName and name have been sorted out, I have pushed up a much simpler fix for the redirect bug in #8505 . Thanks to @basil 's comment that made me revisit some of my earlier attempts.

If the simpler fix is accepted, I guess this PR might be considered a proposed UI enhancement? If there is a consensus to continue it, that is.

(reviewed the other change it has an issue that I don't think you can solve with it)

github-actions · 2023-10-13T15:23:15Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

car-roll · 2023-11-17T19:11:38Z

closing in favor of #8505

Fix redirect when submitting cloud changes

081cadd

car-roll commented Jul 28, 2023

View reviewed changes

core/src/main/java/hudson/slaves/Cloud.java Outdated Show resolved Hide resolved

jglick reviewed Jul 28, 2023

View reviewed changes

car-roll changed the title ~~Fix redirect when submitting cloud changes~~ [JENKINS-71737] Fix redirect when submitting cloud changes Jul 29, 2023

update comment

cb2a5fd

car-roll marked this pull request as ready for review July 29, 2023 06:28

jglick requested a review from Vlatombe August 11, 2023 16:30

test cloud name change

1741953

timja requested changes Aug 12, 2023

View reviewed changes

core/src/main/java/hudson/slaves/Cloud.java Outdated Show resolved Hide resolved

car-roll marked this pull request as draft August 15, 2023 06:06

add separate page for cloud rename

a0a3e9c

car-roll force-pushed the redirect-cloud-name-change branch from c1d02fe to a0a3e9c Compare August 15, 2023 06:19

car-roll added 3 commits August 15, 2023 10:14

tests cloud name change

3d50865

block using apply in cloud rename

29ad035

fix comments

ff3cbba

car-roll closed this Aug 15, 2023

car-roll reopened this Aug 15, 2023

Merge branch 'master' into redirect-cloud-name-change

dbf61fd

timja added the regression-fix Pull request that fixes a regression in one of the previous Jenkins releases label Aug 15, 2023

Remove rename change, block apply when changing name

108bc22

car-roll marked this pull request as ready for review August 16, 2023 19:17

car-roll requested review from timja and mawinter69 August 16, 2023 19:18

Revert "Remove rename change, block apply when changing name"

cc7c4e1

This reverts commit 108bc22.

add cloudName back to formData

1024b9e

car-roll closed this Sep 12, 2023

car-roll reopened this Sep 12, 2023

car-roll added 2 commits September 12, 2023 09:08

cleanup

4b89c49

add name fields to config form data

8c3d6c6

This was referenced Sep 13, 2023

[JENKINS-71737] refactor to support dedicated cloud rename page jenkinsci/kubernetes-plugin#1432

Closed

[JENKINS-71737] make MesosCloud name uneditable jenkinsci/mesos-plugin#420

Closed

car-roll mentioned this pull request Sep 13, 2023

[JENKINS-71737] update for dedicated cloud rename page jenkinsci/ec2-plugin#891

Closed

basil reviewed Sep 13, 2023

View reviewed changes

core/src/main/java/jenkins/agents/CloudSet.java Outdated Show resolved Hide resolved

car-roll mentioned this pull request Sep 13, 2023

[JENKINS-71737] rename cloudName to name jenkinsci/ec2-plugin#892

Merged

remove cloudName from new cloud form data

231db94

car-roll mentioned this pull request Sep 14, 2023

[JENKINS-71737] rename cloudName to name jenkinsci/azure-vm-agents-plugin#462

Merged

basil reviewed Sep 15, 2023

View reviewed changes

car-roll mentioned this pull request Sep 19, 2023

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

Merged

save to disk when changing name

13bab6c

car-roll closed this Sep 19, 2023

car-roll reopened this Sep 19, 2023

Merge branch 'master' into redirect-cloud-name-change

e933c51

github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 13, 2023

car-roll closed this Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

car-roll commented Jul 28, 2023 •

edited

Loading

Submitter checklist

Maintainer checklist

jglick left a comment

jtnord commented Jul 28, 2023

basil commented Jul 31, 2023

car-roll commented Aug 15, 2023

car-roll commented Aug 16, 2023 •

edited

Loading

Vlatombe commented Aug 17, 2023 •

edited

Loading

timja commented Aug 17, 2023

car-roll commented Sep 13, 2023

car-roll commented Sep 13, 2023

car-roll commented Sep 15, 2023

basil Sep 15, 2023

car-roll Sep 19, 2023

car-roll Sep 19, 2023

car-roll Sep 19, 2023

basil Sep 19, 2023 •

edited

Loading

basil Sep 15, 2023

car-roll Sep 19, 2023

basil Sep 19, 2023

basil Sep 15, 2023

basil Sep 15, 2023

car-roll Sep 19, 2023 •

edited

Loading

car-roll commented Sep 19, 2023 •

edited

Loading

timja commented Sep 19, 2023

github-actions bot commented Oct 13, 2023

car-roll commented Nov 17, 2023

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

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

Conversation

car-roll commented Jul 28, 2023 • edited Loading

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

jglick left a comment

Choose a reason for hiding this comment

jtnord commented Jul 28, 2023

basil commented Jul 31, 2023

car-roll commented Aug 15, 2023

car-roll commented Aug 16, 2023 • edited Loading

Vlatombe commented Aug 17, 2023 • edited Loading

timja commented Aug 17, 2023

car-roll commented Sep 13, 2023

car-roll commented Sep 13, 2023

car-roll commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

car-roll Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

car-roll commented Sep 19, 2023 • edited Loading

timja commented Sep 19, 2023

github-actions bot commented Oct 13, 2023

car-roll commented Nov 17, 2023

car-roll commented Jul 28, 2023 •

edited

Loading

car-roll commented Aug 16, 2023 •

edited

Loading

Vlatombe commented Aug 17, 2023 •

edited

Loading

basil Sep 19, 2023 •

edited

Loading

car-roll Sep 19, 2023 •

edited

Loading

car-roll commented Sep 19, 2023 •

edited

Loading