JENKINS-42854: Added description field to the 'Computer' api #2807
Conversation
|
FWIW I'd much prefer a more description-like description (with formatting and all), rather than the weird one we currently have. I looked into this a few years back and it wasn't straightforward, don't remember why unfortunately. Could have been related to assumptions in the description widget? |
oleg-nenashev
added
the
needs-more-reviews
istrangiu
force-pushed
the
AddApiNodeDescription
branch
from
ec1f770
to
638b19e
Compare
|
@daniel-beck Could you please elaborate?I don't really understand what you mean. Many thanks. |
istrangiu
force-pushed
the
AddApiNodeDescription
branch
2 times, most recently
from
d7841cb
to
f44e236
Compare
| @Exported | ||
| public @Nonnull String getDescription() { | ||
| Node node = getNode(); | ||
| return (node != null) ? node.getNodeDescription() : ""; |
There was a problem hiding this comment.
Maybe better to return null
There was a problem hiding this comment.
@oleg-nenashev Please could you explain to me why do you think is better to return null instead of an empty string?Thank you in advance.
There was a problem hiding this comment.
It is a public API, and without "null" API users may be confused, because empty description is not a undefined description. It is not that important anyway
|
I suppose @daniel-beck is concerned about the current design of Jenkins where descriptions have a limited functionality. Once and if description becomes a formatted text, returning it via REST API will become not that user-friendly |
|
@daniel-beck gentle ping |
|
Assigned to @daniel-beck since the PR is kinda blocked by his comment |
|
I consider review from @daniel-beck as dismissed, no response after the month. @jenkinsci/code-reviewers please provide the feedback |
|
Last time I looked into this I couldn't get node descriptions to work, IIRC due to the field/property name which is assumed to be 'description'. I worry whether this change will result in additional problems moving to a real node description, but haven't had the time to investigate. In particular, if this is only for the remote API, |
istrangiu
force-pushed
the
AddApiNodeDescription
branch
from
f44e236
to
6f53766
Compare
|
@oleg-nenashev @daniel-beck Sorry for the late reply, I have implemented your feedback. Please review the latest changes. |
oleg-nenashev
added
ready-for-merge
|
@istrangiu thanks for your efforts and patience! |
Description
When creating a new slave node it is possible to add a description, however the description is not available when querying the api for the list of nodes.
See JENKINS-42854.
Details:
I have exposed the description field of a Slave Node that was previously missing in the api.
I have not provided new unit tests as the patch is trivial, also I have not found existing tests for that area of code.
Changelog entries
Proposed changelog entries:
Submitter checklist
Desired reviewers
@jglick @kohsuke @stephenc