Introduce a new summary description field for jobs #4489

oleg-nenashev · 2020-02-11T15:00:34Z

Draft implementation for my suggestion in #4477 which could be a solution which is acceptable to both sides (or not)

Proposed changelog entries

Bug: Reintroduce Build History truncation by default, allow managing it via a system property
Developer: Add support of a summary field for runs

Proposed upgrade guidelines

N/A

Submitter checklist

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). 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
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 correct
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 should exist and be labeled as lts-candidate

…] fixes for limiting the build history widget Reverts jenkinsci#4209

timja

seems like the most pragmatic approach and what will satisfy the most users

timja · 2020-02-11T15:04:16Z

core/src/main/java/hudson/model/Run.java

+    @Exported
+    @CheckForNull
+    public String getSummary() {
+        return summary != null ? summary : getTruncatedDescription();


do you need to check if it's empty, or will it always be safely null here?

I would prefer a strict data model here (good for getter performance), but I can add String truncation on the setter side

timja · 2020-02-11T15:04:37Z

core/src/main/java/hudson/model/Run.java

     * @return The length-limited description.
-     * @deprecated truncated description uses arbitrary and unconfigurable limit of 100 symbols
+     * @deprecated truncated description based on the {@link #TRUNCATED_DESCRIPTION_LIMIT} setting.
     */
    @Deprecated


should this be un deprecated?

I think no, because getSummary is a proper API.

Yes, I would prefer to not change the state here.

I mentioned it as this PR adds documentation for it:
https://github.com/jenkinsci/jenkins/pull/4489/files#diff-c4f9931d88bca347279b881007d71f0eR234

Probably shouldn't be suggesting to call a deprecated API?

when compiling, we will have a warning because we are calling a deprecated api. I don't have solution, just worse mentioning that this is intentional, no?

slonopotamus

I like this, assuming it will be possible to set summary via pipeline scripts. There are some bugs, though, thus marking PR as 'request changes'.

Additionally, you possibly want to allow editing of summary field on "Edit Build Information" page.

core/src/main/java/hudson/model/Run.java

core/src/main/resources/hudson/widgets/HistoryWidget/entry.jelly

core/src/main/java/hudson/model/Run.java

oleg-nenashev · 2020-02-11T15:56:30Z

FTR introduction of a proper summary field is going to be an uphill battle. Jenkins core only includes the AbstractBuild implementation for UI, it will need updates in Pipeline plugins & Co

oleg-nenashev · 2020-02-11T15:58:27Z

core/src/main/java/hudson/model/Run.java

@@ -659,20 +679,42 @@ public final long getStartTimeInMillis() {
        return startTime;
    }

+    //TODO(oleg_nenashev): Do we want to export full description by default taking the size considerations? Looks like it should be another REST API or a flag


I wonder whether @harry-g and @jsoref use REST APIs for builds. The current state is a potentially big problem for long descriptions

Why would anything change for REST API? getDescription always returned full text.

That's why I do not touch it. But I believe that it is not ideal for users who are concerned about huge REST APIs

No, I think we don't use it afaik.
But I would also discourage delivering the full description with the build object.
(Where it is only one build run, so less severe than the current job/build with/config page which load description from tons of builds.)

Just decrease visibility if you don't like it here. People should be requesting it via ?tree anyway.

More of a separate change though.

…uncation was introduced eons ago

oleg-nenashev · 2020-02-11T16:46:40Z

[2020-02-11T16:25:25.753Z] [INFO] --- spotbugs-maven-plugin:3.1.12:check (spotbugs) @ jenkins-core ---

[2020-02-11T16:25:25.753Z] [INFO] BugInstance size is 1

[2020-02-11T16:25:25.753Z] [INFO] Error size is 0

[2020-02-11T16:25:25.753Z] [INFO] Total bugs: 1

[2020-02-11T16:25:25.753Z] [ERROR] Possible null pointer dereference of Run.description in hudson.model.Run.getTruncatedDescription() [hudson.model.Run, hudson.model.Run] Dereferenced at Run.java:[line 730]Known null at Run.java:[line 730] NP_NULL_ON_SOME_PATH

[2020-02-11T16:25:25.753Z] [INFO]

harry-g · 2020-02-11T17:25:32Z

Just my 5 ct: the summary field would be nice if starting from scratch.
But with so many pipelines to modify for all users and also side effects like pipeline DSL itself and maybe some plugins depending on it, I think it is probably not the best way to go.

harry-g · 2020-02-11T17:34:04Z

Ups, sorry, I overlooked that the summary takes the truncated description if empty. Just thought that this could be the solution, then saw it is already in your change :-D
So pipelines do not need to be modified.
But as you said yourself

it will need updates in Pipeline plugins & Co

res0nance · 2020-02-13T08:50:20Z

FTR introduction of a proper summary field is going to be an uphill battle. Jenkins core only includes the AbstractBuild implementation for UI, it will need updates in Pipeline plugins & Co

If we add something to https://github.com/jenkinsci/workflow-support-plugin/blob/master/src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/RunWrapper.java

We will be able to set it from pipeline with currentBuild.summary = "Some summary"

But it's an uphill battle due to having to bump core unless you use reflection :/

slonopotamus · 2020-02-13T09:46:29Z

This reminds me of the "Epic Story of Adding a Boolean Field to git-plugin": jenkinsci/git-plugin#792. 3 weeks, 15 PR iterations, 56 comments, 16 changed files and 200 lines of changes. Things should not be that much complex.

jsoref

I have no particular opinions about this.

I'm not a particular fan of having HTML magic here. If someone uses a different formatter, is there a reasonable way for this magic to work?

jsoref · 2020-02-13T16:15:04Z

core/src/main/java/hudson/model/Run.java

-    public @Nonnull String getTruncatedDescription() {
-        final int maxDescrLength = 100;
-        if (description == null || description.length() < maxDescrLength) {
+    public @CheckForNull String getTruncatedDescription() {


Wouldn't it be stylistically better to do:

@CheckForNull public String ...

It would. Just following the historical codestyle

oleg-nenashev · 2020-02-14T10:57:04Z

@jsoref

I'm not a particular fan of having HTML magic here. If someone uses a different formatter, is there a reasonable way for this magic to work?

Me too, but this is the current state of affairs. We could add a new truncate() method to the markupFormatter extension so that the implementations can override it

oleg-nenashev · 2020-02-15T11:41:36Z

Test failure is valid

  18.807 [id=76]	WARNING	o.e.j.s.h.ContextHandler$Context#log: Error while serving http://localhost:36647/jenkins/job/test0/1/configSubmit
net.sf.json.JSONException: JSONObject["summary"] not found.
	at net.sf.json.JSONObject.getString(JSONObject.java:2040)
	at hudson.model.Run.submit(Run.java:2545)
	at hudson.model.Run.doConfigSubmit(Run.java:2536)
	at java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:627)
	at org.kohsuke.stapler.Function$MethodFunction.invoke(Function.java:396)

oleg-nenashev · 2020-02-17T13:45:39Z

The Sunday's weekly is getting delayed, so maybe we could try to get this into the weekly

daniel-beck · 2020-02-19T00:09:55Z

This one snuck up on me. Could I get a few more days to review?

oleg-nenashev · 2020-02-19T09:20:03Z

@daniel-beck sure

fcojfernandez · 2020-02-20T10:18:22Z

core/src/main/java/hudson/model/Run.java

+     */
+    @Exported
+    @CheckForNull
+    public String getSummary() {


Should it have a suppressed warning for the use of a deprecated method?

fcojfernandez

+1 for this proposal. IMO, this new summary field should have been in the model since the beginning

alecharp

We will be adding a deprecation warning at compile time, but it's not a big issue.

alecharp · 2020-02-20T10:21:20Z

core/src/main/java/hudson/model/Run.java

     * @return The length-limited description.
-     * @deprecated truncated description uses arbitrary and unconfigurable limit of 100 symbols
+     * @deprecated truncated description based on the {@link #TRUNCATED_DESCRIPTION_LIMIT} setting.
     */
    @Deprecated


when compiling, we will have a warning because we are calling a deprecated api. I don't have solution, just worse mentioning that this is intentional, no?

daniel-beck · 2020-02-20T14:08:33Z

+1 for this proposal. IMO, this new summary field should have been in the model since the beginning

It's called the build name.

oleg-nenashev · 2020-02-22T19:30:04Z

@daniel-beck do you still want to do a review? It is likely to miss the weekly. Looks like I will need to create a backportable version of a fix without new API

daniel-beck · 2020-02-22T22:06:21Z

do you still want to do a review?

Yes.

It is likely to miss the weekly.

🤷‍♂

Not every open PR needs to go into 2.222. After #4506, #4365, #4463, and #4501, it's a busy enough release.

daniel-beck · 2020-02-24T02:19:34Z

@oleg-nenashev FWIW I consider jsoref#5 to be a nice standalone change, if you want something to make the LTS release less broken with long descriptions.

daniel-beck · 2020-02-24T02:32:03Z

Why doesn't the summary field show up on (build)/configure or on (build)/? It exists in the API and (indirectly) side-panel only, and that makes it really difficult to even discover. The new API endpoint is not documented on (build)/api either.

There's also the weird API model that makes it impossible for short descriptions to tell whether an identical summary is set, or whether the summary is just the description. I guess this is to make the sidepanel code easier, but is just confusing.

There's also a bug where submitting (build)/configure for a build that has a summary set wipes the summary.

I'm undecided whether the entire summary concept makes sense to me -- see #4489 (comment) -- but this implementation has problems.

daniel-beck

As above.

oleg-nenashev · 2020-02-27T09:37:51Z

Why doesn't the summary field show up on (build)/configure or on (build)/?

Suggested changelog documents it as a developer change. I do not plan to work on the rest of frontend until we coordinate delivery for Pipeline. #4489 (comment)

There's also a bug where submitting (build)/configure for a build that has a summary set wipes the summary

It is. I will probably detach the bugfix and the rest for now

oleg-nenashev · 2020-02-28T08:16:21Z

New version is here: #4529
It removes the build summary concept

varyvol · 2020-09-03T09:44:21Z

#4529 was merged. So is this still necessary?

alecharp · 2020-10-01T13:37:18Z

@oleg-nenashev shouldn't we close this PR as #4529 was merged? Thanks.

oleg-nenashev · 2020-10-01T15:46:00Z

Unfortunately, no plans to work on it anytime soon

…

On Thu, Oct 1, 2020, 15:37 Adrien Lecharpentier ***@***.***> wrote: @oleg-nenashev <https://github.com/oleg-nenashev> shouldn't we close this PR as #4529 <#4529> was merged? Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4489 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAW4RIFB5EX3GGE5UHSOSZTSISA2BANCNFSM4KTCNPSA> .

alecharp · 2020-10-15T13:09:11Z

should we close this PR for now and you can re-open it when you have more time?

alecharp · 2020-11-12T14:31:49Z

I'm closing this PR for now, please reopen it when you will be able to work on it again.

oleg-nenashev added 2 commits February 11, 2020 15:50

Introduce the Run summary API, revert [JENKINS-19760], [JENKINS-31209…

f3313d2

…] fixes for limiting the build history widget Reverts jenkinsci#4209

Allow managing the build history limit via system properties

a1e34c9

oleg-nenashev added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Feb 11, 2020

oleg-nenashev requested review from slonopotamus and jsoref February 11, 2020 15:00

oleg-nenashev mentioned this pull request Feb 11, 2020

[JENKINS-61004] - Default limits for buildHistoryPage row heights #4477

Closed

10 tasks

timja reviewed Feb 11, 2020

View reviewed changes

slonopotamus suggested changes Feb 11, 2020

View reviewed changes

core/src/main/java/hudson/model/Run.java Outdated Show resolved Hide resolved

core/src/main/resources/hudson/widgets/HistoryWidget/entry.jelly Outdated Show resolved Hide resolved

core/src/main/java/hudson/model/Run.java Outdated Show resolved Hide resolved

I will make sure to add some tests...

9bf0605

oleg-nenashev commented Feb 11, 2020

View reviewed changes

Run#getTruncatedDescription() was always able to return null since tr…

8161545

…uncation was introduced eons ago

slonopotamus approved these changes Feb 11, 2020

View reviewed changes

Prevent race condition between description modification and truncation

1003cc6

jsoref reviewed Feb 13, 2020

View reviewed changes

oleg-nenashev marked this pull request as ready for review February 14, 2020 10:59

Summary is optional

523d296

oleg-nenashev added bug For changelog: Minor bug. Will be listed after features developer Changes which impact plugin developers labels Feb 16, 2020

oleg-nenashev requested a review from a team February 18, 2020 11:07

timja approved these changes Feb 18, 2020

View reviewed changes

markjacksonfishing approved these changes Feb 18, 2020

View reviewed changes

oleg-nenashev added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Feb 19, 2020

timja removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 20, 2020

fcojfernandez reviewed Feb 20, 2020

View reviewed changes

fcojfernandez approved these changes Feb 20, 2020

View reviewed changes

alecharp approved these changes Feb 20, 2020

View reviewed changes

daniel-beck added the plugin-api-changes Changes the API of Jenkins available for use in plugins. label Feb 24, 2020

daniel-beck requested changes Feb 24, 2020

View reviewed changes

daniel-beck added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Feb 24, 2020

oleg-nenashev mentioned this pull request Feb 28, 2020

[JENKINS-61004, JENKINS-60299] - Reintroduce Build History description truncation by default, allow managing/disabling the limit via a system property #4529

Merged

10 tasks

oleg-nenashev added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted and removed bug For changelog: Minor bug. Will be listed after features labels Mar 5, 2020

oleg-nenashev changed the title ~~[JENKINS-61004, JENKINS-60299] - Reintroduce Build History truncation by default, allow managing it via a system property + Summary field~~ Introduce a new summary description field for jobs Mar 5, 2020

alecharp added the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 1, 2020

rsandell added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Nov 6, 2020

alecharp closed this Nov 12, 2020

Introduce a new summary description field for jobs #4489

Introduce a new summary description field for jobs #4489

Conversation

oleg-nenashev commented Feb 11, 2020 • edited by MarkEWaite Loading

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

timja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slonopotamus left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Feb 11, 2020

harry-g commented Feb 11, 2020

harry-g commented Feb 11, 2020

res0nance commented Feb 13, 2020 • edited Loading

slonopotamus commented Feb 13, 2020

jsoref left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Feb 14, 2020

oleg-nenashev commented Feb 15, 2020

oleg-nenashev commented Feb 17, 2020

daniel-beck commented Feb 19, 2020

oleg-nenashev commented Feb 19, 2020

Choose a reason for hiding this comment

fcojfernandez left a comment

Choose a reason for hiding this comment

alecharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck commented Feb 20, 2020

oleg-nenashev commented Feb 22, 2020

daniel-beck commented Feb 22, 2020

daniel-beck commented Feb 24, 2020

daniel-beck commented Feb 24, 2020

daniel-beck left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Feb 27, 2020

oleg-nenashev commented Feb 28, 2020

varyvol commented Sep 3, 2020

alecharp commented Oct 1, 2020

oleg-nenashev commented Oct 1, 2020 via email

alecharp commented Oct 15, 2020

alecharp commented Nov 12, 2020

oleg-nenashev commented Feb 11, 2020 •

edited by MarkEWaite

Loading

res0nance commented Feb 13, 2020 •

edited

Loading