-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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-60866][JENKINS-71797] Remove timeline widget #6869
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, is there anything remaining to take this out of draft?
See also discussion on the forum:
https://community.jenkins.io/t/does-anybody-find-the-build-history-timeline-useful/8491/5
Unsure why this was draft to begin with. I think I wanted to prevent premature merges without giving people the opportunity to weigh in, but I guess that is obsolete since about a year ago? :-) Working on #8431 I discovered this has some leftover stuff in bom etc. that I still need to clean up. |
Noting https://github.com/jenkinsci/jqs-monitoring-plugin/blob/f50a82a0bf323c647c68aadd9eda9590a9468c2d/src/main/java/org/jenkinsci/plugins/jqsmonitoring/slaves/SlavesHolder.java#L109 / https://github.com/jenkinsci/jqs-monitoring-plugin/blob/f50a82a0bf323c647c68aadd9eda9590a9468c2d/src/main/java/org/jenkinsci/plugins/jqsmonitoring/slaves/SlavesHolder.java#L124 as API user who will not be affected by this change (other than a |
Might even fix (or alleviate somewhat) https://issues.jenkins.io/browse/JENKINS-72138 |
Could you run this through ATH and bom please to be safe? Happy to approve after that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
+1 from me to remove the widget. That should resolve the JavaScript warning that I see in the browser console when viewing the timeline. The message in the console is:
|
/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! |
See JENKINS-60866.
This removes the timeline widget without replacement. I've never found it particularly useful, and it is a problem for CSP. We probably want something different for these views, but for now, remove without replacement.
Screenshots
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgrade@Restricted
or have@since TODO
Javadoc, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
if applicable.Desired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are accurate, human-readable, and in the imperative moodupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate
to be considered (see query).