jenkins icon indicating copy to clipboard operation
jenkins copied to clipboard

Colored node labels

Open mPokornyETM opened this issue 2 years ago • 20 comments

See JENKINS-XXXXX.

New page with all known 'node labels' image todo: I know it the font is for some reason broken, but I does not know why. Will check it little later.

Labels in node view: image

Label view: image

Label config page: image

Proposed changelog entries

  • Colored node labels
  • Show all existing labels

Proposed upgrade guidelines

N/A

Submitter checklist

  • [ ] (If applicable) Jira issue is well described
  • [ ] Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • [ ] Appropriate autotests or explanation to why this change has no tests
  • [ ] New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • [ ] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

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

  • [ ] There are at least 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 does not block the change
  • [ ] Changelog entries in the PR 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, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (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).

mPokornyETM avatar Jul 09 '22 21:07 mPokornyETM

When this gets merged, #6498 is probably obsolete

mawinter69 avatar Jul 10 '22 09:07 mawinter69

Currently there is no other way of providing a new link in the ComputerSet page other than directly in core. So maybe it makes sense to have some kind of ComputerSetAction as extension point. This would allow to provide this functionality in a plugin. I would also be interested in such an extension point.

mawinter69 avatar Jul 10 '22 10:07 mawinter69

When this gets merged, #6498 is probably obsolete

not rely. I think this can be good combinate together. Or make something like this: image

image

image

and configure page: image

But I think it will be much more easy to make a plugin for that, that allow more features in node monitoring (I am currently working on extend-node-monitoring plugin and need this PR for that)

mPokornyETM avatar Jul 11 '22 08:07 mPokornyETM

Testing this PR does not work for me, on any agent page after adding a label:

org.apache.commons.jelly.JellyTagException: jar:file:/root/.jenkins/war/WEB-INF/lib/jenkins-core-2.359-SNAPSHOT.jar!/hudson/model/Computer/index.jelly:83:81: <st:include> No page found '../Label/Badge/index.jelly' for class hudson.model.labels.LabelAtom

daniel-beck avatar Jul 11 '22 19:07 daniel-beck

Testing this PR does not work for me, on any agent page after adding a label:

org.apache.commons.jelly.JellyTagException: jar:file:/root/.jenkins/war/WEB-INF/lib/jenkins-core-2.359-SNAPSHOT.jar!/hudson/model/Computer/index.jelly:83:81: <st:include> No page found '../Label/Badge/index.jelly' for class hudson.model.labels.LabelAtom

strange, checked on my side now, and it works. I will debug it later

mPokornyETM avatar Jul 12 '22 06:07 mPokornyETM

Solely from a UI/accessibility perspective, I think it'd be better to limit the potential colors to only those supported by the Jenkins Palette - https://weekly.ci.jenkins.io/design-library/Colors/

This way the colors can change depending on theme and it'd be harder for a user to pick a color that isn't readable etc.

janfaracik avatar Jul 12 '22 15:07 janfaracik

It works for GitHub, why wouldn't it work for Jenkins?

All (well, way more than ~20) colors are readable if the label color is generated with enough contrast to the chosen background color.

daniel-beck avatar Jul 12 '22 16:07 daniel-beck

About themes and limited count of colors. That was my first try. But like @daniel-beck mentioned in github it works as well, so I has changed it to simple color-picker. So the administrator needs select useful colors. And we can set little border over label depend on theme.

mPokornyETM avatar Jul 12 '22 16:07 mPokornyETM

Currently there is no other way of providing a new link in the ComputerSet page other than directly in core. So maybe it makes sense to have some kind of ComputerSetAction as extension point. This would allow to provide this functionality in a plugin. I would also be interested in such an extension point.

I can try that. Bur not sure if I can do that 🤔. We will see.

mPokornyETM avatar Jul 12 '22 17:07 mPokornyETM

Testing this PR does not work for me, on any agent page after adding a label:

org.apache.commons.jelly.JellyTagException: jar:file:/root/.jenkins/war/WEB-INF/lib/jenkins-core-2.359-SNAPSHOT.jar!/hudson/model/Computer/index.jelly:83:81: <st:include> No page found '../Label/Badge/index.jelly' for class hudson.model.labels.LabelAtom

Same here, I'm not able to visit the page either. Full stacktrace

I think it'd be better to limit the potential colors to only those supported by the Jenkins Palette - https://weekly.ci.jenkins.io/design-library/Colors/

This way the colors can change depending on theme and it'd be harder for a user to pick a color that isn't readable etc.

A middle way could be used to recommend and advertise the usage of predefined "native Jenkins" colors you can click to select, which have theming support, but keeping up the flexibility to define your own colors, if desired.

NotMyFault avatar Jul 16 '22 14:07 NotMyFault

Testing this PR does not work for me, on any agent page after adding a label:

org.apache.commons.jelly.JellyTagException: jar:file:/root/.jenkins/war/WEB-INF/lib/jenkins-core-2.359-SNAPSHOT.jar!/hudson/model/Computer/index.jelly:83:81: <st:include> No page found '../Label/Badge/index.jelly' for class hudson.model.labels.LabelAtom

Same here, I'm not able to visit the page either. Full stacktrace

I do not understand it. I have deleted whole war/target directory and rebuild it again and it works (on my side :-) ). So when I understand the stack-trace correctly:

org.apache.commons.jelly.JellyTagException: jar:file:/Users/alex/.jenkins/war/WEB-INF/lib/jenkins-core-2.360-SNAPSHOT.jar!/hudson/model/Computer/index.jelly:83:81: <st:include> No page found '../Label/Badge/index.jelly' for class hudson.model.labels.LabelAtom

It tries to find Label/Badge/index.jelly but it didn find. The file is committed and exist also in war file: image

maybe are my build options 'wrong' mvn -am -pl war,bom -DskipTests -Dspotbugs.skip -Dspotless.check.skip -Dcheckstyle.failOnViolation=false clean install && mvn -pl war jetty:run

Developed && tested on Windows. But I also do not see caseSensitive failure for unix paths.

Can somebody help me please?

mPokornyETM avatar Jul 30 '22 10:07 mPokornyETM

I think it'd be better to limit the potential colors to only those supported by the Jenkins Palette - https://weekly.ci.jenkins.io/design-library/Colors/

This way the colors can change depending on theme and it'd be harder for a user to pick a color that isn't readable etc.

A middle way could be used to recommend and advertise the usage of predefined "native Jenkins" colors you can click to select, which have theming support, but keeping up the flexibility to define your own colors, if desired.

This is very good idea. @janfaracik is there any picker for jenkins-colors? Before I start with the implementation? Or a programmatic way to get all supported colors (css variables)

mPokornyETM avatar Jul 30 '22 10:07 mPokornyETM

Given recent discussions in this PR it seems like there are nontrivial changes pending, so I'll hold off on another round of security review for now.

daniel-beck avatar Aug 12 '22 13:08 daniel-beck

This is very good idea. @janfaracik is there any picker for jenkins-colors? Before I start with the implementation? Or a programmatic way to get all supported colors (css variables)

There isn't a picker sadly/nor a programatic way to get the colours. You'd have to hardcode the list sadly, the list of available colours in Jenkins (with vars + classes) is here https://weekly.ci.jenkins.io/design-library/Colors/

janfaracik avatar Aug 15 '22 17:08 janfaracik

Testing this PR does not work for me, on any agent page after adding a label:

org.apache.commons.jelly.JellyTagException: jar:file:/root/.jenkins/war/WEB-INF/lib/jenkins-core-2.359-SNAPSHOT.jar!/hudson/model/Computer/index.jelly:83:81: <st:include> No page found '../Label/Badge/index.jelly' for class hudson.model.labels.LabelAtom

After making sure the PR builds if you skip tests (triaging SpotBugs is still up to do), I run into the issue still. There's no difference if I use a fresh or regular setup, the core PR tester or launching via the command line.

I recommend to pause further reviews until this is resolved, given core functionality cannot be warranted in the current state, hence I'll mark the PR as draft.

Feel free to change that once the PR is ready to be reviewed and tested again.

NotMyFault avatar Aug 23 '22 20:08 NotMyFault

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

github-actions[bot] avatar Sep 09 '23 03:09 github-actions[bot]

Interesting conflict. @janfaracik How do you suggest the merge conflict be resolved here? Seems awkward to have one "configuration" option in the app bar, and the other in the side panel. But how many items can the app bar realistically hold before it looks silly?

daniel-beck avatar Sep 09 '23 10:09 daniel-beck

Sorry guys, I really forgot to continue here. Hopefully will found time in next feature to continue here.

mPokornyETM avatar Sep 12 '23 07:09 mPokornyETM

OK, I recheckt it again, After a "little" pause I recognized, that there are few things to do. I will probably close this PR, and provide few smaller PRs

mPokornyETM avatar Nov 27 '23 10:11 mPokornyETM