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

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

Merged

Conversation

oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev commented Feb 28, 2020

This is a backportable version of #4489 which does not introduce the new summary field. See JENKINS-61004 and JENKINS-60299.

Proposed changelog entries

  • bug: Reintroduce Build History description truncation by default. Allow managing/disabling the limit via the historyWidget.descriptionLimit system property (negative value - remove the limit, 0 - force empty descriptions)
  • ...

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

… default, allow managing/disabling the limit via a system property
@oleg-nenashev oleg-nenashev added the bug For changelog: Minor bug. Will be listed after features label Feb 28, 2020
@oleg-nenashev oleg-nenashev changed the title [JENKINS-61004] - Reintroduce Build History description truncation by default, allow managing/disabling the limit via a system property [JENKINS-61004, JENKINS-60299] - Reintroduce Build History description truncation by default, allow managing/disabling the limit via a system property Feb 28, 2020
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.

one nit, but approving

return description;
}
if (TRUNCATED_DESCRIPTION_LIMIT == 0) { // Someone wants to suppress descriptions, why not?
Copy link
Member

Choose a reason for hiding this comment

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

personally I would make 0 disable truncation as 0 often means unlimited, why introduce a feature to disable descriptions? has anyone asked for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That was 2 possible solutions, didn't suggest implementing both 😉

Choose a reason for hiding this comment

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

Hi, just wanted to chime in. I'm the one who created JENKINS-60299. For our use-case a possibility to disable the build description in the build history widget would be really nice. Note though that we very much need the build description for each build, just not in the widget itself.
Off course, I'm just one user so I understand if our specific need cannot be taken into account and implemented as a feature for all users :-)

Copy link
Member

Choose a reason for hiding this comment

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

🤷‍♂ fine with me then.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like a reasonable option for people using non-HTML markup formatting, which is horrifically broken by the server-side, pre-transformation truncation.

@oleg-nenashev oleg-nenashev requested a review from a team February 28, 2020 13:17
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.

Reviewed but not tested. Minor suggestions, would consider them nice but non-blocking.

Let's be explicit that this won't be the final state but rather a stop gap, and 🚢 🇮🇹

* @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.
Copy link
Member

Choose a reason for hiding this comment

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

The deprecation doesn't really make sense anymore? At least for a while?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not want to revert it for now, will probably act on that in a follow-up PR

*/
@Deprecated
public @Nonnull String getTruncatedDescription() {
Copy link
Member

Choose a reason for hiding this comment

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

How was this ever @Nonnull? 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

🤷‍♂

@oleg-nenashev
Copy link
Member Author

Taking the approvals, I plan to merge it tomorrow if no negative feedback

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 28, 2020
@oleg-nenashev
Copy link
Member Author

ATH is stuck due to the infra issues. I will ignore it and get it merged

@harry-g
Copy link

harry-g commented Mar 11, 2020

Tested OK in 2.225 by setting up a test job with 10 runs and huuuge html descriptions (see JENKINS-60299).
Thanks a lot @oleg-nenashev for driving it forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants