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-72978] Remove executors widget from Dashboard and nodes page #9126
base: master
Are you sure you want to change the base?
Conversation
This is indeed pretty neat. Would it be useful, for a limited period of time, to display a link in the position of the current executor widget, that takes you to the new overview? |
I think changelog is probably ok, I've added a link to the nodes page for low privileged users as there was no way for them to get there now |
Maybe we can just remove the Factories for views |
Makes sense
What's the reason to reuse it on the status page? Its current setup assumes its in the sidepanel and likely needs some changes to make it fit in properly, I've done a minimal bit so that it looks okay for now though. |
Yeah I wasn't sure if we wanted that or not, can add it though if worth it. There's a smaller risk of people leaving it open forever and lots of polling updates.
Yeah styling is a bit messed up though I had to make some minor changes to the markup to make it work like removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, not really a blocker but something to think about
/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! |
I'd like an opportunity to review :) Thanks! |
What's the intended level of done-ness of this PR? This looks like a reasonable first step and implements the high-level request, but doesn't go much further than that. For example jenkins/core/src/main/java/hudson/model/View.java Lines 158 to 161 in c660d27
@Deprecated , transient and Boolean .
Some less clear interaction issues:
IMO most of the information being presented should remain accessible to users who administer Jenkins. I don't think anyone cares about which # executor a build is running on, but the rest seems interesting. The widget on the computer view answers the question, which job is currently using this node; while the new Status page does the same across all nodes (like the widget did before) and e.g. tells admins how far a potential "quiet down" has been successful. Thoughts about delivering this as a UI experiment that users can opt out of, just in case? (Then of course without removal of (FTR I'm trying to find the time to look into moving the widget into a popup, like admin monitors, with some similarity to the redesign prototype.) |
What would be also helpful is if one could collapse the executors of a single agent like you can collapse the complete widget |
Basically what I mentioned as
The specific executor assignment wasn't even useful information when it was in the sidepanel, only used/total would be relevant IMO. But that's an easy followup (or even independent parallel) change to make. |
created #9177 with the idle executors removed |
Minor nitpick/not blocking - the widget is inset a little bit |
See JENKINS-72978.
Implements https://matrix.to/#/!HKutvjxPnajVyCNLhF:matrix.org/$kfLAa91tXfp6Gv10PgoY8i7GM3GgqfDG_pz39-nWsDQ?via=gitter.im&via=matrix.org&via=mozilla.org
Adds a replacement page with the same information. I thought about redesigning it but I didn't think it made sense to do in this pull request.
This could fairly easily be detached to a plugin if people really want this still.
Testing done
manually checked the widget doesn't exist anymore except on the computer page
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist
upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).