[JENKINS-72020] Restore ability to reorder clouds #8492

Vlatombe · 2023-09-15T14:06:18Z

Adding back controls to reorder clouds.

Since clouds order is taken into account for node provisioning, users should be able to reorder them as needed.

Enregistrement.de.l.ecran.2023-10-09.a.18.06.15.mov

See JENKINS-72020.

Testing done

Proposed changelog entries

Allow clouds to be reordered. This was previously possible, but disappeared when the cloud management was moved to a separate page (regression in 2.403).

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

Adding back controls to reorder clouds. Since clouds order is taken into account for node provisioning, users should be able to reorder them as needed.

NotMyFault · 2023-09-16T06:50:04Z

core/src/main/resources/jenkins/agents/CloudSet/index.jelly

+                        <a href="move" class="jenkins-table__button">
+                          <input type="hidden" name="name" value="${cloud.name}"/>
+                          <j:if test="${not status.first}">
+                            <f:submit name="up" value="" icon="symbol-chevron-up"/>


I think a tertiary or button with a symbol would look less intrusive, especially if you have multiple clouds set up: https://weekly.ci.jenkins.io/design-library/Buttons/

I updated the PR with tertiary buttons. Let me know what you think!

timja · 2023-09-20T15:30:30Z

(Still not a huge fan of the UX, I've pulled it locally and when I get a chance in the next day or two hopefully I can propose something better)

Vlatombe · 2023-09-20T15:38:21Z

Thanks @timja ! Let me know if you find something better.

core/src/main/resources/jenkins/agents/CloudSet/reorder.properties

Vlatombe · 2023-09-25T08:08:10Z

Getting further, I included your first option and a save button. Still not fully satisfied with the save usability though.

timja · 2023-09-25T08:09:29Z

Getting further, I included your first option and a save button. Still not fully satisfied with the save usability though.

you could probably call submit the form async in some way in the on complete, or have the save button made visible after re-arranging?

Not the best as it seems a bit weird to have a save button on this page

core/src/main/resources/jenkins/agents/CloudSet/index.jelly

Only display order column when admin

war/src/main/js/pages/cloud-set/index.js

timja · 2023-09-25T16:12:02Z

core/src/main/resources/jenkins/agents/CloudSet/index.jelly

+              <thead>
+                <tr>
+                  <l:isAdmin>
+                    <th class="jenkins-table__cell--tight">Order</th>


Looking at the video this seems a bit mis-aligned, we could possibly drop the label in favour of a tooltip on the handle?

Should be slightly better now.

Vlatombe · 2023-09-27T07:51:49Z

What do you think about this?

I prefer the version where the handle is on the left.

The other option might be to just drop the cloud icon as well?

I think once cloud implementations catch up with providing proper icons this will improve the page usability.

Also considered making the icon the drag & drop handle (like you would in a file explorer) but doesn't seem trivial with current dd component.

timja · 2023-09-27T07:58:35Z

I think once cloud implementations catch up with providing proper icons this will improve the page usability.

It's not really about catching up it's licensing, at least with Azure it's damn hard to get the license -.-.
Neither myself or people in Microsoft managed to get anywhere for the Azure plugins...

timja · 2023-09-27T07:59:11Z

I also prefer it on the left, but double icons doesn't feel right to me
cc @janfaracik if you have any input

Vlatombe · 2023-10-02T15:00:03Z

I merged the two columns, the cloud icon can now be used as a drag-and-drop handle.

timja

Works for me

NotMyFault

🚀

timja · 2023-10-03T09:30:50Z

/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!

MarkEWaite · 2023-10-05T10:45:24Z

@Wadeck would the security team like to review this before merge?

Wadeck · 2023-10-05T11:04:14Z

@MarkEWaite Yes please, I think we have forgotten to add a comment / label at this point :(

daniel-beck

Requesting change for the vulnerability.

As a general note, it's not obvious why cloud order matters (and in the current iteration, how to change it, as noted in a line comment). Ideally above or below the clouds list, the impact of order on behavior should be explained.

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

daniel-beck · 2023-10-06T10:26:22Z

core/src/main/resources/jenkins/agents/CloudSet/index.jelly

+                      <div class="jenkins-table__cell__button-wrapper dd-handle">
+                        <l:icon src="${cloud.iconClassName}" tooltip="${cloud.iconAltText}"/>
+                      </div>


IMO: This was nicer as in the video, it's too difficult to discover currently.

core/src/main/resources/jenkins/agents/CloudSet/index.jelly

security issue was addressed

timja · 2023-10-10T09:30: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!

timja · 2023-10-12T08:03:00Z

IMO: This was nicer as in the video, it's too difficult to discover currently.

This was discussed at the UX sig yesterday, https://docs.google.com/document/d/1QttPwdimNP_120JukigKsRuBvMr34KZhVfsbgq1HFLM/edit?pli=1

Christina is going to try come up with a design for a next iteration.
We agreed the issue is non blocking.

MarkEWaite · 2023-10-29T13:36:25Z

Should this be backported into the 2.426.1 LTS release?

timja · 2023-10-29T19:24:11Z

Yes makes sense

@timja

* [JENKINS-72020] Reorder clouds Adding back controls to reorder clouds. Since clouds order is taken into account for node provisioning, users should be able to reorder them as needed. * Use tertiary buttons * Separate screen for reordering * Include reordering in the cloud list directly (thanks @timja) * Show the save button after the first reordering * Separate JS Only display order column when admin * Fix reviews * Remove unused string * Put drag-and-drop handle in the cloud icon column * Add missing permission check * Add a small blurb to clarify what cloud order is used for. (cherry picked from commit 16bca2e)

[JENKINS-72020] Reorder clouds

e2e0dcf

Adding back controls to reorder clouds. Since clouds order is taken into account for node provisioning, users should be able to reorder them as needed.

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

Vlatombe marked this pull request as ready for review September 15, 2023 15:38

NotMyFault reviewed Sep 16, 2023

View reviewed changes

Use tertiary buttons

85912c3

Vlatombe requested a review from NotMyFault September 18, 2023 08:00

NotMyFault approved these changes Sep 18, 2023

View reviewed changes

Vlatombe requested a review from a team September 18, 2023 16:31

This comment was marked as outdated.

Sign in to view

Separate screen for reordering

daf1343

This comment was marked as outdated.

Sign in to view

NotMyFault self-requested a review September 20, 2023 14:03

Vlatombe requested a review from timja September 20, 2023 15:06

This comment was marked as outdated.

Sign in to view

timja added web-ui The PR includes WebUI changes which may need special expertise needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging labels Sep 24, 2023

timja reviewed Sep 24, 2023

View reviewed changes

core/src/main/resources/jenkins/agents/CloudSet/reorder.properties Outdated Show resolved Hide resolved

Include reordering in the cloud list directly (thanks @timja)

e040db8

Show the save button after the first reordering

9d2b2cd

timja reviewed Sep 25, 2023

View reviewed changes

core/src/main/resources/jenkins/agents/CloudSet/index.jelly Outdated Show resolved Hide resolved

Separate JS

4525b5c

Only display order column when admin

Vlatombe requested a review from timja September 25, 2023 13:57

timja reviewed Sep 25, 2023

View reviewed changes

war/src/main/js/pages/cloud-set/index.js Outdated Show resolved Hide resolved

timja reviewed Sep 25, 2023

View reviewed changes

Fix reviews

002b773

Vlatombe requested a review from timja September 26, 2023 08:08

Put drag-and-drop handle in the cloud icon column

c315e94

timja approved these changes Oct 2, 2023

View reviewed changes

NotMyFault removed the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Oct 3, 2023

NotMyFault approved these changes Oct 3, 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 Oct 3, 2023

Wadeck added the needs-security-review Awaiting review by a security team member label Oct 5, 2023

MarkEWaite removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 5, 2023

daniel-beck previously requested changes Oct 6, 2023

View reviewed changes

Vlatombe added 2 commits October 9, 2023 18:00

Add missing permission check

3daa05c

Add a small blurb to clarify what cloud order is used for.

3da4c36

Vlatombe requested a review from daniel-beck October 9, 2023 16:05

daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Oct 9, 2023

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 Oct 10, 2023

NotMyFault merged commit 16bca2e into jenkinsci:master Oct 11, 2023
16 checks passed

Vlatombe deleted the reorder-clouds-JENKINS-72020 branch October 12, 2023 07:20

MarkEWaite mentioned this pull request Oct 29, 2023

Backporting for 2.426.1 #8658

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-72020] Restore ability to reorder clouds #8492

[JENKINS-72020] Restore ability to reorder clouds #8492

Vlatombe commented Sep 15, 2023 •

edited by MarkEWaite

Loading

Submitter checklist

Maintainer checklist

NotMyFault Sep 16, 2023

Vlatombe Sep 18, 2023

NotMyFault Sep 18, 2023

This comment was marked as outdated.

This comment was marked as outdated.

timja commented Sep 20, 2023

Vlatombe commented Sep 20, 2023

This comment was marked as outdated.

Vlatombe commented Sep 25, 2023

timja commented Sep 25, 2023

timja Sep 25, 2023 •

edited

Loading

Vlatombe Sep 26, 2023

Vlatombe commented Sep 27, 2023

timja commented Sep 27, 2023

timja commented Sep 27, 2023

Vlatombe commented Oct 2, 2023 •

edited

Loading

timja left a comment

NotMyFault left a comment

timja commented Oct 3, 2023

MarkEWaite commented Oct 5, 2023

Wadeck commented Oct 5, 2023

daniel-beck left a comment •

edited

Loading

daniel-beck Oct 6, 2023

timja commented Oct 10, 2023

timja commented Oct 12, 2023

MarkEWaite commented Oct 29, 2023

timja commented Oct 29, 2023

[JENKINS-72020] Restore ability to reorder clouds #8492

[JENKINS-72020] Restore ability to reorder clouds #8492

Conversation

Vlatombe commented Sep 15, 2023 • edited by MarkEWaite Loading

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

NotMyFault Sep 16, 2023

Choose a reason for hiding this comment

Vlatombe Sep 18, 2023

Choose a reason for hiding this comment

NotMyFault Sep 18, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

timja commented Sep 20, 2023

Vlatombe commented Sep 20, 2023

This comment was marked as outdated.

Vlatombe commented Sep 25, 2023

timja commented Sep 25, 2023

timja Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Vlatombe Sep 26, 2023

Choose a reason for hiding this comment

Vlatombe commented Sep 27, 2023

timja commented Sep 27, 2023

timja commented Sep 27, 2023

Vlatombe commented Oct 2, 2023 • edited Loading

timja left a comment

Choose a reason for hiding this comment

NotMyFault left a comment

Choose a reason for hiding this comment

timja commented Oct 3, 2023

MarkEWaite commented Oct 5, 2023

Wadeck commented Oct 5, 2023

daniel-beck left a comment • edited Loading

Choose a reason for hiding this comment

daniel-beck Oct 6, 2023

Choose a reason for hiding this comment

timja commented Oct 10, 2023

timja commented Oct 12, 2023

MarkEWaite commented Oct 29, 2023

timja commented Oct 29, 2023

Vlatombe commented Sep 15, 2023 •

edited by MarkEWaite

Loading

timja Sep 25, 2023 •

edited

Loading

Vlatombe commented Oct 2, 2023 •

edited

Loading

daniel-beck left a comment •

edited

Loading