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

Allow formatted markup for node descriptions #6511

Merged
merged 34 commits into from
Jan 2, 2023

Conversation

mPokornyETM
Copy link
Contributor

@mPokornyETM mPokornyETM commented Apr 23, 2022

See JENKINS-XXXXX.

Proposed changelog entries

  • Allow HTML syntax for node descriptions.
    Before
    image

After
image

Proposed upgrade guidelines

No

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
    There are no changes in java code.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
    There are no changes in java code.
  • For dependency updates: links to external changelogs and, if possible, full diffs
    There are no changes in java code.

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 mPokornyETM changed the title markup formatted description for node Allow markup formatted description for node Apr 23, 2022
@NotMyFault NotMyFault changed the title Allow markup formatted description for node Allow HTML syntax for node descriptions Apr 23, 2022
@NotMyFault NotMyFault added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted squash-merge-me Unclean or useless commit history, should be merged only with squash-merge labels Apr 23, 2022
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

This is a nice approach, but I'd recommend to stick to what's already in place.
For description input we're using a codemirror box, like seen on job or user descriptions, with optional syntax support, if enabled in the security realm.

Elevating a jenkins-input to be able to render HTML breaks with the existing handling for these kinds of text input.

@mPokornyETM
Copy link
Contributor Author

I try to find some example. There is editableDescrption page but it does not work on this page. Because this page has an extra config page. Has you some idea?

@NotMyFault
Copy link
Member

I try to find some example. There is editableDescrption page but it does not work on this page. Because this page has an extra config page. Has you some idea?

Changing the input from a textbox to a textarea in hudson.slaves.DumbSlave.configure-entries.jelly pointing the markup formatter to codemirror does generate named box.
Existing spots are for example hudson.model.Job.configure.jelly utilizing this feature.

@mPokornyETM
Copy link
Contributor Author

So editable config description works too.

image
-->
image

The button to 'swicht temporally offline' is moved too. So it has the same look like in configure job page.
@NotMyFault thanks for hits.

@daniel-beck
Copy link
Member

How does this look in matrix-project? I think that renders node descriptions and should be adapted.

Did you check whether project history explains why nodes have plain text nodeDescription but everything else has formatted description?

@mPokornyETM
Copy link
Contributor Author

mPokornyETM commented Apr 26, 2022

How does this look in matrix-project? I think that renders node descriptions and should be adapted.
No I have no idea, newer use it, but I will try it and get feedback here

Did you check whether project history explains why nodes have plain text nodeDescription but everything else has formatted description?

Nope, I was just angry, because we can not use longer description. We want to fill the description dynamically to get some node specific info when is started / used. Ex:

  • installed tools and their versions like git, java ...
  • real platform name (the platform labeler plugin does not work as expected for us)
  • IpConfig
  • Reboot history, we need to reboot the nodes many times during our 'build' process
    ....

So we need bigger description field.

But I will try to find the reason. It was also terrible for me to fix it, because you true anywhere else it is description

@mPokornyETM
Copy link
Contributor Author

matrix-project

So the matrix-project renders node labels. Node name and description are not changed.
image
image
I think my changes has no effect on this.

@mPokornyETM
Copy link
Contributor Author

Did you check whether project history explains why nodes have plain text nodeDescription but everything else has formatted description?

So I have try to find the reason, It looks that it was just "for ever". When you agree, I would create new Jira issue for that. So we can "fix" it little later. I prefer to set the 'nodeDescription' to obsolete, but this can not be decided by me.

@mPokornyETM
Copy link
Contributor Author

I found one more usage of nodeDescription
image

I has also check whole jenkins core ws for nodeDescription and I think this was the last one.

@daniel-beck
Copy link
Member

@mPokornyETM Thanks! To clarify, I'm not opposed to this change per se, just wondering whether there's a good reason for the discrepancy in both field name and behavior. I expect we'll merge this or a variant in the end, solving your problem.

Re matrix-project, I was surprised by your answer and checked, and while

Screenshot 2022-04-27 at 13 33 58

looks like there would be an agent description rendered there, there's no support for that -- everything is treated as a label, not as an agent name, internally: https://github.com/jenkinsci/matrix-project-plugin/blob/7aea491852f39b51379a2f6f2dd490f7d191441e/src/main/java/hudson/matrix/LabelAxis.java#L90-L99
Additionally, the label descriptions do not get the renderer applied, so it looks terrible with any non-default markup formatter. That's an issue with the plugin alone though.

@mPokornyETM
Copy link
Contributor Author

@daniel-beck Ah sorry that was misunderstanding from my side. I think it renders some hove the /computer/<agentName> page, but it is used here in the config page from the matrix-project self.
The solution is strange. From my side I prefer to remove the description from this page, because it is not necessary and it looks like the lol () is a function and not the node name (description)

Please allow me one question. What shall be done to finish this PR?

@daniel-beck
Copy link
Member

Please allow me one question. What shall be done to finish this PR?

At the moment, needs more reviews from others, and consequently some patience from you. If anything is missing from this PR, that'll be brought up in reviews.

@daniel-beck
Copy link
Member

daniel-beck commented Apr 27, 2022

Nit:

The description of the built-in node is defined by Hudson.NodeDescription in hudson/model/Messages.properties as

the Jenkins controller's built-in node

That looks weird with this change in presentation:

Screenshot 2022-04-27 at 18 02 20

It should be upgraded to a full sentence since it's no longer a short phrase in parentheses (no HTML though, or any characters likely to lead to problem in any common(ish) markup language).

@mPokornyETM
Copy link
Contributor Author

Please allow me one question. What shall be done to finish this PR?

At the moment, needs more reviews from others, and consequently some patience from you. If anything is missing from this PR, that'll be brought up in reviews.

thanks, I am patient enough, but I am excited, Like a small child

@mPokornyETM
Copy link
Contributor Author

Nit:

The description of the built-in node is defined by Hudson.NodeDescription in hudson/model/Messages.properties as

the Jenkins controller's built-in node

That looks weird with this change in presentation:

Screenshot 2022-04-27 at 18 02 20

It should be upgraded to a full sentence since it's no longer a short phrase in parentheses (no HTML though, or any characters likely to lead to problem in any common(ish) markup language).

Hm I see. The same issue was in System Information page. My quick and dirty solution was
<div style="margin-bottom: var(--section-padding);">

Do you now about some better class or html element, which will render the space correctly?

@mPokornyETM
Copy link
Contributor Author

dirty fix for padding
image

@daniel-beck
Copy link
Member

It should be upgraded to a full sentence since it's no longer a short phrase in parentheses (no HTML though, or any characters likely to lead to problem in any common(ish) markup language).

Hm I see. The same issue was in System Information page. My quick and dirty solution was <div style="margin-bottom: var(--section-padding);">

To clarify, the comment you quoted was about the phrasing, not the presentation.

Thanks for taking care of the margin!

@daniel-beck daniel-beck self-requested a review April 28, 2022 15:51
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 25, 2022
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 25, 2022
@basil
Copy link
Member

basil commented Aug 25, 2022

I merged the physical conflicts but now there is a logical conflict:

core/src/main/resources/lib/hudson/editable-description.js
   9:16  error  'replaceDescription' is not defined  no-undef
  11:16  error  'replaceDescription' is not defined  no-undef

@basil basil mentioned this pull request Aug 25, 2022
@mPokornyETM
Copy link
Contributor Author

@daniel-beck thx for caring.

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.

@mPokornyETM Sorry for the delay here!

TBH I'm trying to figure out what is blocking this. I got assigned after providing early feedback. I provided a suggestion addressing early problems, which you integrated. That had open TODOs that needed resolving, but those have been resolved and the TODOs removed.

Is it just that I didn't unassign myself, so nobody else went near this? I'll try to see whether changing that gets others to take a look 😃 (as a substantial co-author I'm unsure whether I should be approving this.)

@daniel-beck daniel-beck removed their assignment Dec 15, 2022
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Dec 29, 2022
@github-actions
Copy link
Contributor

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

@timja timja self-requested a review December 29, 2022 20:16
@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Dec 31, 2022
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

LGTM


There's one UI issue but likely pre-existing issue with the component / other buttons on the page.

The Save and mark this node temporarily offline buttons don't look that great as save is wider than the form and they are touching

image

@timja timja requested a review from a team December 31, 2022 11:38
@timja
Copy link
Member

timja commented Dec 31, 2022

/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 Dec 31, 2022
@timja timja merged commit 5522f60 into jenkinsci:master Jan 2, 2023
@mPokornyETM mPokornyETM deleted the feature/nodeDescription branch January 10, 2023 06:12
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 squash-merge-me Unclean or useless commit history, should be merged only with squash-merge 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.

7 participants