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.

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-70729] Rework clouds management into multiple pages #7658

Merged
merged 56 commits into from
Apr 28, 2023

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Feb 17, 2023

cf. the idea I described in https://groups.google.com/g/jenkinsci-dev/c/71eTUHSFE9I/m/J4Jzx6kwAgAJ

  • Separate entry under Manage Jenkins
  • One configuration page per cloud
  • One index page per cloud

Here are a few screenshots

Details

Capture d’écran 2023-02-16 à 12 02 03

image image image image image image image

See JENKINS-70729.

Testing done

Created clouds of type:

Updated clouds
Deleted clouds

Installed downstream PR for Azure which demonstrates overriding the icon: jenkinsci/azure-vm-agents-plugin#383

Proposed changelog entries

  • Rework clouds management into multiple pages to better scale to large numbers of clouds. Users of EC2 Plugin should update it to version 2.0.7 or newer for compatibility.

Proposed upgrade guidelines

N/A

Submitter checklist

  • 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.

Desired reviewers

@mention

Maintainer checklist

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

  • 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).

* Separate entry under Manage Jenkins
* One configuration page per cloud
* One index page per cloud
@Vlatombe Vlatombe added the work-in-progress The PR is under active development, not ready to the final review label Feb 17, 2023
@timja
Copy link
Member

timja commented Mar 4, 2023

Looks really nice, the only cloud I could get to actually work currently is one from mock-slave.
but excited for this and happy to help on it.

(pushed one quick fix for layout)

@timja
Copy link
Member

timja commented Mar 4, 2023

Adding descriptor to the request made all the plugins views now load on create.
and I was also able to save the Kubernetes plugin now.

Can't save Azure or EC2. Both fail with:

Query parameter 'name' is required

@Vlatombe
Copy link
Member Author

Vlatombe commented Mar 4, 2023

Can't save Azure or EC2. Both fail with:
Query parameter 'name' is required

Should work now. EC2 has an odd behaviour because the persisted name differs from the name displayed in the UI.

@timja timja added work-in-progress The PR is under active development, not ready to the final review rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted needs-security-review Awaiting review by a security team member and removed work-in-progress The PR is under active development, not ready to the final review needs-security-review Awaiting review by a security team member labels Mar 4, 2023
@timja timja removed the work-in-progress The PR is under active development, not ready to the final review label Mar 5, 2023
@timja timja marked this pull request as ready for review March 5, 2023 15:33
@timja timja requested review from a team March 5, 2023 15:33
@timja timja added the needs-security-review Awaiting review by a security team member label Mar 5, 2023
@timja
Copy link
Member

timja commented Mar 5, 2023

It looks good to me.

I think only thing left is to look at the empty state when there's no clouds.

I'm not 100% sold on the table, there's not much being displayed there but I don't think we have anything better. Anyway it's a lot better than before.

ATH being checked here: jenkinsci/acceptance-test-harness#1047

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Why does this not adapt /configureClouds/, instead creating a separate page? If that's deliberate, why does that page remain, essentially duplicating the UI, and why does it remain linked from the empty All view shown after installation, as well as the admin monitor recommending setup of an agent or cloud?

Was a UI similar to plugin manager's "Available" and "Uploads" being different sections of the same top-level page considered, with "Manage Clouds" one page and "Manage Nodes" the other? If yes, what are the drawbacks compared to completely separate top-level items? Given that clouds create a subset of agents, completely separating them again this way (not even having direct navigation between them) seems very awkward.

The "New Cloud" UI is very awkward when there are no clouds installed, very much unlike /configureClouds/ (which replaces the regular UI with a helpful message). This should be reviewed.

Downstream PRs to cloud implementations like Kubernetes should be filed as needed, if they adapted their UI to show only few fields by default, to limit initial complexity in the potentially giant list (screenshot in PR description).

Permissions checks seem way off, need lots more validation to ensure this works as intended.

@Vlatombe
Copy link
Member Author

Vlatombe commented Apr 26, 2023

There are remaining plugins that do not support custom names (listed above). For these plugins, the user experience is the following:

  • when creating such a cloud, the user entered name is ignored. Instead the name computed by the plugin is used. A warning is reported in such a case. The same is applicable if you copy an existing cloud.
  • When encountering a cloud with a duplicate name, they are accessed by their index instead of their name to allow consistent editing, and deletion.

With that, I think we don't need to keep a fallback UI even for plugins that may not have been updated to support user provided names.

@timja
Copy link
Member

timja commented Apr 26, 2023

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

@comment-ops-bot 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 Apr 26, 2023
@jglick jglick removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 26, 2023
@timja timja requested a review from jglick April 27, 2023 10:46
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Not attempting to review this in its entirety, just a buglet I noticed. Fine to restore ready-for-merge so far as I am concerned.

@timja
Copy link
Member

timja commented Apr 27, 2023

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

@comment-ops-bot 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 Apr 27, 2023
@timja timja merged commit 4963094 into jenkinsci:master Apr 28, 2023
Vlatombe added a commit to Vlatombe/jenkins that referenced this pull request May 9, 2023
@Vlatombe Vlatombe mentioned this pull request May 9, 2023
14 tasks
NotMyFault pushed a commit that referenced this pull request May 10, 2023
Support clouds without a config.jelly

Follows up #7658

For consistency with 45e656c
@basil
Copy link
Member

basil commented Jun 6, 2023

Causes JENKINS-71371.

@basil
Copy link
Member

basil commented Aug 14, 2023

Causes JENKINS-71825.

MarkEWaite added a commit to MarkEWaite/docker-plugin that referenced this pull request Sep 3, 2023
jenkinsci/jenkins#7658 reworks cloud management
to use multiple pages instead of a single large page.  That rework also
requires that a cloud may not be named with an empty string.

The docker plugin specifically tested that a cloud was allowed to have an
empty string as its name, but performed no other operations with a cloud
that is named with an empty string.  Cloud named with an empty string may
have worked prior to Jenkins 2.403, but they would have been complicated
to manage, especially in a controller with multiple clouds defined.

A cloud named with an empty string is much more difficult to manage.
It is better to mention in the changelog that clouds are no longer
allowed to have an empty name rather than wrestling with all the ways
that an empty name will break cloud administration.

https://issues.jenkins.io/browse/JENKINS-70729 is the enhancement request
that implements the change.

https://issues.jenkins.io/browse/JENKINS-71825 is the bug report for
the automated test failure that started this investigation.
MarkEWaite added a commit to jenkinsci/docker-plugin that referenced this pull request Sep 4, 2023
jenkinsci/jenkins#7658 reworks cloud management
to use multiple pages instead of a single large page.  That rework also
requires that a cloud may not be named with an empty string.

The docker plugin specifically tested that a cloud was allowed to have an
empty string as its name, but performed no other operations with a cloud
that is named with an empty string.  Cloud named with an empty string may
have worked prior to Jenkins 2.403, but they would have been complicated
to manage, especially in a controller with multiple clouds defined.

A cloud named with an empty string is much more difficult to manage.
It is better to mention in the changelog that clouds are no longer
allowed to have an empty name rather than wrestling with all the ways
that an empty name will break cloud administration.

https://issues.jenkins.io/browse/JENKINS-70729 is the enhancement request
that implements the change.

https://issues.jenkins.io/browse/JENKINS-71825 is the bug report for
the automated test failure that started this investigation.
jtnord added a commit to jtnord/google-compute-engine-plugin that referenced this pull request May 2, 2024
amends jenkinsci#173 due to jenkinsci/jenkins#7658

bumps the baseline to avoid the redirect or adding a TODO
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants