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

Rewrite the build history widget #9148

Merged
merged 54 commits into from
Jun 14, 2024

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Apr 9, 2024

This PR implements the changes discussed https://community.jenkins.io/t/revamped-build-history-widget/10944. In essence it's a rewrite/redesign of the build history widget. The goal is to be cleaner and simpler, whilst still retaining existing functionality and density.

Before
image
image
image

After
image

image image image

What's changed

  • The JS has been rewritten and does away with a lot of the complicated layout logic in favour of CSS flex/grid.
  • A new card component has been added (with the intention of replacing the existing widget component) that supports inline actions
  • Builds now group by date, keeping the UI simpler/easier to read
  • RSS feed has been moved to a dropdown, rather than a fixed button row
  • Build overflow menus are now always present on the right of the link

What doesn't work

  • Pagination controls are always visible at the moment
  • The list doesn't auto refresh at the moment
  • Items with long names/lots of actions don't scale very well

Testing done

  • See Rewrite the build history widget #9148 (comment) for a view of the scenarios tested
    • Normal scenarios work as expected (e.g. short build names, one or two badges)
    • Queued items look as expected
    • Items with plain text/HTML descriptions work as expected
    • Items with long names/lots of badges wrap as expected

Proposed changelog entries

  • Update the design of the build history widget.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@jenkinsci/sig-ux

Before the changes are marked as ready-for-merge:

Maintainer checklist

commit 1ce4cb3
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue Apr 9 09:22:36 2024 +0100

    Rename classes

commit 107d794
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue Apr 9 08:59:54 2024 +0100

    Update HistoryWidget.java

commit 13575bc
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue Apr 9 08:59:29 2024 +0100

    Hide buttons

commit e5de546
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue Apr 9 08:39:04 2024 +0100

    Rename classes

commit 47c84af
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 8 23:21:36 2024 +0100

    Add animation

commit e7432b6
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 8 23:03:19 2024 +0100

    Add navigation buttons

commit 448a094
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 8 22:29:32 2024 +0100

    Update _dashboard.scss

commit bb85734
Merge: c451a72 27433f1
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 8 22:23:38 2024 +0100

    Merge branch 'master' into new-build-history-2

commit c451a72
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 8 13:17:05 2024 +0100

    Update _job.scss

commit 960b162
Merge: d020eb6 af655e3
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 8 13:16:54 2024 +0100

    Merge branch 'remove-table-usage' into new-build-history-2

commit af655e3
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 8 13:13:38 2024 +0100

    Init

commit d020eb6
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 8 13:09:57 2024 +0100

    Update card.jelly

commit 81bc1d4
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 8 13:07:41 2024 +0100

    Update _buttons.scss

commit 4023460
Merge: 875fb8f da5f593
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Apr 8 13:03:18 2024 +0100

    Merge branch 'master' into new-build-history-2

commit 875fb8f
Merge: 2dc9964 fe60fac
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Apr 6 19:29:20 2024 +0100

    Merge branch 'master' into new-build-history-2

commit 2dc9964
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Apr 5 22:17:08 2024 +0100

    Reset files

commit 2da1e14
Merge: dce466a b9fac75
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Apr 5 22:14:46 2024 +0100

    Merge branch 'master' into new-build-history-2

commit dce466a
Merge: 935c16e 0eed048
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Feb 28 13:55:35 2024 +0000

    Merge branch 'master' into new-build-history-2

commit 935c16e
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Feb 18 17:54:32 2024 +0000

    Update entry.jelly

commit 2a19a04
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Feb 18 17:51:58 2024 +0000

    Rename classes

commit 3ef46ab
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Feb 18 17:29:25 2024 +0000

    Update index.jelly

commit da0b212
Merge: 281fac0 9d9e2ab
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Feb 18 17:27:12 2024 +0000

    Merge branch 'revamp-dropdowns' into new-build-history-2

commit 281fac0
Merge: 9c69bd0 a642354
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Feb 18 17:19:14 2024 +0000

    Merge branch 'master' into new-build-history-2

commit 9d9e2ab
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Feb 10 19:39:24 2024 +0000

    Update templates.js

commit 1c19e43
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Feb 10 19:38:13 2024 +0000

    Add clazz

commit 8b944e9
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Feb 10 15:19:30 2024 +0000

    Update utils.js

commit 069fefb
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Feb 10 13:23:16 2024 +0000

    Linting

commit 7270712
Merge: 9865811 a642354
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Feb 10 12:20:02 2024 +0000

    Merge branch 'master' into revamp-dropdowns

commit 9865811
Merge: 1e22c34 86d39dd
Author: Mark Waite <mark.earl.waite@gmail.com>
Date:   Thu Jan 18 05:39:13 2024 -0700

    Merge branch 'master' into revamp-dropdowns

commit 9c69bd0
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jan 14 13:56:49 2024 +0000

    Push

commit 347e966
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jan 14 13:53:52 2024 +0000

    Update filter-build-history.js

commit 0b4a5dd
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jan 14 13:49:54 2024 +0000

    Renam

commit a8277bf
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jan 14 13:46:53 2024 +0000

    Fix

commit 855bf13
Merge: 61b0a87 1eb29a8
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jan 14 13:38:51 2024 +0000

    Merge branch 'master' into new-build-history-2

commit 1e22c34
Merge: 44981c2 48661db
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Jan 14 13:33:14 2024 +0000

    Merge branch 'master' into revamp-dropdowns

commit 44981c2
Merge: 0075375 1eb29a8
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Jan 8 21:14:51 2024 +0000

    Merge branch 'master' into revamp-dropdowns

commit 0075375
Merge: 2dd9e32 78cdaa9
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jan 4 13:26:24 2024 +0000

    Merge branch 'master' into revamp-dropdowns

commit 2dd9e32
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jan 4 13:24:53 2024 +0000

    Remove translations

commit 6800c88
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jan 4 13:16:19 2024 +0000

    Update header.jelly

commit 1c3961b
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Jan 4 13:15:49 2024 +0000

    Add additional docs

commit 163be52
Merge: 4cc43e4 444f2de
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Jan 3 21:22:20 2024 +0000

    Merge branch 'master' into revamp-dropdowns

commit 61b0a87
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Dec 14 19:38:45 2023 +0000

    More

commit dcd6aaa
Merge: 0c40b9f edce488
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Dec 14 19:38:25 2023 +0000

    Merge branch 'stop-button' into new-build-history-2

commit edce488
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Dec 14 16:06:00 2023 +0000

    Tidy up

commit 157ba0b
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Dec 14 16:04:46 2023 +0000

    Fix i18n

commit a112bd9
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Dec 13 22:57:17 2023 +0000

    Update _buttons.scss

commit 91751cf
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Dec 13 22:39:18 2023 +0000

    Update executors.jelly

commit cd89aea
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Dec 13 22:31:06 2023 +0000

    Fixes

commit 1384091
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Dec 13 22:28:54 2023 +0000

    Init

commit 0c40b9f
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Dec 13 16:50:53 2023 +0000

    Update _buttons.scss

commit 50fe8fc
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Wed Dec 13 13:13:10 2023 +0000

    add view transitions

commit d1fd7a9
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue Dec 12 20:25:44 2023 +0000

    Tidy up

commit 512b9a1
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Tue Dec 12 19:29:16 2023 +0000

    Push

commit 4f15690
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Dec 11 23:30:09 2023 +0000

    push

commit ad75b0f
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Dec 11 23:24:11 2023 +0000

    Update _buttons.scss

commit cec222e
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Dec 11 23:23:52 2023 +0000

    Update _buttons.scss

commit 4cc43e4
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Dec 10 16:04:28 2023 +0000

    Add docs

commit a4c7f4f
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Dec 10 16:00:23 2023 +0000

    Update taglib

commit c01db44
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Dec 10 15:56:50 2023 +0000

    Init

commit 21bb3f4
Merge: d3e2920 428d0e5
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Dec 10 13:22:00 2023 +0000

    Merge branch 'restyle-cards' into new-build-history-2

commit 428d0e5
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Dec 7 20:17:10 2023 +0000

    Lower weight

commit ac5c255
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Dec 7 20:15:28 2023 +0000

    Remove more bold weights

commit 2922a69
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Dec 7 20:11:33 2023 +0000

    Update _style.scss

commit 9657d46
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Dec 7 20:06:16 2023 +0000

    Init

commit d3e2920
Merge: a2bd9a0 cabc8f6
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Thu Dec 7 19:50:43 2023 +0000

    Merge branch 'master' into new-build-history-2

commit a2bd9a0
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sun Nov 26 11:58:22 2023 +0000

    Fixes

commit 7cda504
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 12:28:04 2023 +0000

    Working build

commit abd994e
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 11:58:42 2023 +0000

    Working build

commit 9b6defb
Merge: 29fcb64 7a0e57e
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 11:51:24 2023 +0000

    Merge branch 'progress-bar-new' into new-build-history-2

commit 29fcb64
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 11:51:10 2023 +0000

    More

commit 2f946d3
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 09:17:06 2023 +0000

    Update _buttons.scss

commit f5474b3
Merge: 00c3879 982bc48
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 09:16:14 2023 +0000

    Merge branch 'use-symbols-for-build-status-new' into new-build-history-2

commit 982bc48
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 09:15:56 2023 +0000

    Fix app bar build status icon being incorrect

commit 00c3879
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 09:12:10 2023 +0000

    Fixes

commit a4960e9
Merge: d28aada c6f5db0
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 08:56:01 2023 +0000

    Merge branch 'use-symbols-for-build-status-new' into new-build-history-2

commit d28aada
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 08:55:18 2023 +0000

    Update _buttons.scss

commit 1d24a19
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Sat Nov 25 08:52:58 2023 +0000

    Init

commit 7a0e57e
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Nov 24 21:19:23 2023 +0000

    More

commit 67d4264
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Nov 24 21:17:09 2023 +0000

    Update _spinner.scss

commit 9befc76
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Nov 24 21:07:55 2023 +0000

    Update _spinner.scss

commit 528b46a
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Nov 24 21:01:06 2023 +0000

    More

commit ea0c487
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Nov 24 20:36:53 2023 +0000

    Init

commit c6f5db0
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Nov 24 17:52:28 2023 +0000

    Fix icon position

commit 18a8407
Merge: aea4d97 a9c34d7
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Nov 24 09:58:54 2023 +0000

    Merge branch 'master' into use-symbols-for-build-status-new

commit aea4d97
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Fri Nov 24 09:58:29 2023 +0000

    Rename ID

commit 5f76f38
Author: Jan Faracik <43062514+janfaracik@users.noreply.github.com>
Date:   Mon Nov 20 16:00:14 2023 +0000

    Init
@daniel-beck daniel-beck marked this pull request as draft April 9, 2024 09:17
@timja
Copy link
Member

timja commented Apr 9, 2024

can you add before screenshots too please

@timja timja added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise labels Apr 9, 2024
@mawinter69
Copy link
Contributor

How does this look when a build has a display name?
How does it look when a build has many badges?
Just an example how this looks currently:
image

@janfaracik
Copy link
Contributor Author

How does this look when a build has a display name? How does it look when a build has many badges? Just an example how this looks currently: image

image

It's okay but could be better - I think moving the badges onto a new row when its incredibly long would improve it.

@mawinter69
Copy link
Contributor

What made the previous implementation so big was the logic to find a suitable way to arrange all the things. See #9122 for my PR that changed they calculation to be more efficient.

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 10, 2024
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 Apr 10, 2024
@janfaracik
Copy link
Contributor Author

Getting ReferenceError: "fetch" is not defined. (http://localhost:42617/jenkins/static/8bd93229/jsbundles/pages/project/builds-card.js#52) in tests - have you experienced this before @timja?

@timja
Copy link
Member

timja commented Apr 11, 2024

Getting ReferenceError: "fetch" is not defined. (http://localhost:42617/jenkins/static/8bd93229/jsbundles/pages/project/builds-card.js#52) in tests - have you experienced this before @timja?

I see the issue, PR incoming

@timja
Copy link
Member

timja commented Apr 11, 2024

jenkinsci/jenkins-test-harness#753

@janfaracik
Copy link
Contributor Author

What's concerning to me however is the introduction (AFAIK new here) of a completely new UI for the "context menu". This doesn't use the model-link control anymore, but instead uses an ellipsis-labeled button to open the context menu for the build. This increases inconsistency across Jenkins for what appears to be no real reason. Why?

Downsides to the existing model-link implementation is that it's not immediately visible and that it's less accessible on different device types. The 'overflow' menu introduced as part of this PR is fixed in place, meaning it's predictable and more accessible. The change in iconography I think is also clearer as to what it does and also used in Jenkins already (e.g. with the app bar overflow menu).

@daniel-beck
Copy link
Member

it's not immediately visible and that it's less accessible on different device types

This applies to the entire Jenkins UI. I do not think replacing a single occurrence solves more problems than it introduces.

@janfaracik
Copy link
Contributor Author

it's not immediately visible and that it's less accessible on different device types

This applies to the entire Jenkins UI. I do not think replacing a single occurrence solves more problems than it introduces.

I agree to an extent, but I do thing there is benefit to starting somewhere - with the 'Build history' widget arguably being one of the most used controls in Jenkins it'd be a pretty big win. Any thoughts @jenkinsci/sig-ux ?

@timja
Copy link
Member

timja commented Jun 6, 2024

it's not immediately visible and that it's less accessible on different device types

This applies to the entire Jenkins UI. I do not think replacing a single occurrence solves more problems than it introduces.

I agree to an extent, but I do thing there is benefit to starting somewhere - with the 'Build history' widget arguably being one of the most used controls in Jenkins it'd be a pretty big win. Any thoughts @jenkinsci/sig-ux ?

To me the overflow menu is easier to use in the build history menu like this, and it avoids a number of weird UI issues with placements of the link not being in the right place (although that seems to be more of an issue with the build executor widget)

I don't think we should block the PR on this.

@daniel-beck
Copy link
Member

daniel-beck commented Jun 6, 2024

starting somewhere

I'd like to see at minimum a plan for widespread implementation of this, a proof of concept of sorts. At this point, for me there are too many unanswered questions. How will this work with bread crumbs? List views that contain regularly 3+ such menus on a single line? Consecutive links (Job Name (v) #123 (v))? Links in build logs?

While in isolation this improves the situation for a single control, increasing the inconsistency across the entire UI, without a plan how to get all of Jenkins to adapt, makes it overall net negative for the entire Jenkins UI, especially if we're considering the expectations for the plugin ecosystem to adapt over the long term.

If retaining the flexible-size model-link isn't feasible (although the ones in the build log also don't move surrounding text), could the icon/design be changed to be more closely to a (hovered) model-link?

@janfaracik
Copy link
Contributor Author

starting somewhere

I'd like to see at minimum a plan for widespread implementation of this, a proof of concept of sorts. At this point, for me there are too many unanswered questions. How will this work with bread crumbs? List views that contain regularly 3+ such menus on a single line? Consecutive links (Job Name (v) #123 (v))? Links in build logs?

For sure. There'll be some refinement of the https://jenkins-redesign.vercel.app soon and I'll be sure to include something in that to address the various areas where model-link is used.

If retaining the flexible-size model-link isn't feasible (although the ones in the build log also don't move surrounding text), could the icon/design be changed to be more closely to a (hovered) model-link?

Can do, yes. Like so?

image

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.

Can do, yes. Like so?

Much better, thanks!

@timja
Copy link
Member

timja commented Jun 11, 2024

/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 Jun 11, 2024
@MarkEWaite MarkEWaite merged commit 0419465 into jenkinsci:master Jun 14, 2024
17 checks passed
@janfaracik janfaracik deleted the new-build-history-widget branch June 14, 2024 13:32
@basil
Copy link
Member

basil commented Jun 14, 2024

This changes causes org.jenkinsci.plugins.authorizeproject.strategy.SystemAuthorizationStrategyTest and hudson.plugins.copyartifact.CopyArtifactWorkflowTest to start failing in PCT.

While the failures are relatively straightforward (a lack of fetch method presumably due to an out-of-date test harness, which could be fixed by releasing versions with the latest plugin parent POM), introducing new PCT failures will negatively impact other critical projects and is not something that we can afford.

Since I have detected this before the weekly release has shipped and before the new failures are introduced, I am tentatively planning to revert this on Monday before the next weekly ships. I would rather not do this, since I am pleased with this PR, but as mentioned above the reasoning is to avoid impacting other projects. The amount of effort required to prepare for this change is not terribly high (updating plugin parent POMs in two plugins and adopting the releases in BOM), but it is effort nonetheless and it needs to be done as part of the due diligence of accepting this change.

@basil basil added the pct-fail The plugin-compatibility-test suite needs a forward-compatible change label Jun 14, 2024
@janfaracik
Copy link
Contributor Author

This changes causes org.jenkinsci.plugins.authorizeproject.strategy.SystemAuthorizationStrategyTest and hudson.plugins.copyartifact.CopyArtifactWorkflowTest to start failing in PCT.

While the failures are relatively straightforward (a lack of fetch method presumably due to an out-of-date test harness, which could be fixed by releasing versions with the latest plugin parent POM), introducing new PCT failures will negatively impact other critical projects and is not something that we can afford.

Since I have detected this before the weekly release has shipped and before the new failures are introduced, I am tentatively planning to revert this on Monday before the next weekly ships. I would rather not do this, since I am pleased with this PR, but as mentioned above the reasoning is to avoid impacting other projects. The amount of effort required to prepare for this change is not terribly high (updating plugin parent POMs in two plugins and adopting the releases in BOM), but it is effort nonetheless and it needs to be done as part of the due diligence of accepting this change.

Hey @basil, thanks for raising this!

Happy to fix this (and eager to avoid reverting too) - I haven't worked with PCT before, is there a test output you could link to/is there any guidance around working with it so I can fix it?

@basil
Copy link
Member

basil commented Jun 14, 2024

@janfaracik The problem can be reproduced in authorize-project-plugin outside of PCT. First, run mvn clean install -Pquick-build in Jenkins core (tip of trunk, with the latest unreleased changes), then run the test in authorize-project-plugin with that version of core by running mvn clean verify -Dtest=org.jenkinsci.plugins.authorizeproject.strategy.SpecificUsersAuthorizationStrategyTest#testConfigurationAuthentication -Djenkins.version=2.463-SNAPSHOT (where the last version number is what you compiled in the previous command). It will fail with the same "fetch is not defined" error as PCT. The same should apply to copyartifact-plugin.

In this case at least, it looks like the root cause is that src/test/java/org/jenkinsci/plugins/authorizeproject/testutil/AuthorizeProjectJenkinsRule.java is returning a new WebClient without the fetch polyfill enabled. I confirmed this change fixes it:

diff --git a/src/test/java/org/jenkinsci/plugins/authorizeproject/testutil/AuthorizeProjectJenkinsRule.java b/src/test/java/org/jenkinsci/plugins/authorizeproject/testutil/AuthorizeProjectJenkinsRule.java
index ab30f3e..e63c8f8 100644
--- a/src/test/java/org/jenkinsci/plugins/authorizeproject/testutil/AuthorizeProjectJenkinsRule.java
+++ b/src/test/java/org/jenkinsci/plugins/authorizeproject/testutil/AuthorizeProjectJenkinsRule.java
@@ -56,7 +56,7 @@ public class AuthorizeProjectJenkinsRule extends JenkinsRule {
 
     @Override
     public WebClient createWebClient() {
-        return new WebClient() {
+        WebClient webClient = new WebClient() {
             private static final long serialVersionUID = 3389654318647204218L;
 
             @Override
@@ -68,6 +68,8 @@ public class AuthorizeProjectJenkinsRule extends JenkinsRule {
                 super.throwFailingHttpStatusCodeExceptionIfNecessary(webResponse);
             }
         };
+        webClient.getOptions().setFetchPolyfillEnabled(true);
+        return webClient;
     }
 
     public void before() throws Throwable {

Likely a similar thing needs to be done in copyartifact-plugin. My point, as mentioned in the preceding comment, is not that preparing plugins in this case is particularly difficult, just that it needs to be done in advance of releasing this PR in a core weekly in order to avoid impacting other projects.

@janfaracik
Copy link
Contributor Author

@janfaracik The problem can be reproduced in authorize-project-plugin outside of PCT. First, run mvn clean install -Pquick-build in Jenkins core (tip of trunk, with the latest unreleased changes), then run the test in authorize-project-plugin with that version of core by running mvn clean verify -Dtest=org.jenkinsci.plugins.authorizeproject.strategy.SpecificUsersAuthorizationStrategyTest#testConfigurationAuthentication -Djenkins.version=2.463-SNAPSHOT (where the last version number is what you compiled in the previous command). It will fail with the same "fetch is not defined" error as PCT. The same should apply to copyartifact-plugin.

In this case at least, it looks like the root cause is that src/test/java/org/jenkinsci/plugins/authorizeproject/testutil/AuthorizeProjectJenkinsRule.java is returning a new WebClient without the fetch polyfill enabled. I confirmed this change fixes it:

diff --git a/src/test/java/org/jenkinsci/plugins/authorizeproject/testutil/AuthorizeProjectJenkinsRule.java b/src/test/java/org/jenkinsci/plugins/authorizeproject/testutil/AuthorizeProjectJenkinsRule.java
index ab30f3e..e63c8f8 100644
--- a/src/test/java/org/jenkinsci/plugins/authorizeproject/testutil/AuthorizeProjectJenkinsRule.java
+++ b/src/test/java/org/jenkinsci/plugins/authorizeproject/testutil/AuthorizeProjectJenkinsRule.java
@@ -56,7 +56,7 @@ public class AuthorizeProjectJenkinsRule extends JenkinsRule {
 
     @Override
     public WebClient createWebClient() {
-        return new WebClient() {
+        WebClient webClient = new WebClient() {
             private static final long serialVersionUID = 3389654318647204218L;
 
             @Override
@@ -68,6 +68,8 @@ public class AuthorizeProjectJenkinsRule extends JenkinsRule {
                 super.throwFailingHttpStatusCodeExceptionIfNecessary(webResponse);
             }
         };
+        webClient.getOptions().setFetchPolyfillEnabled(true);
+        return webClient;
     }
 
     public void before() throws Throwable {

Likely a similar thing needs to be done in copyartifact-plugin. My point, as mentioned in the preceding comment, is not that preparing plugins in this case is particularly difficult, just that it needs to be done in advance of releasing this PR in a core weekly in order to avoid impacting other projects.

Big thanks for this - opened merge requests for both plugins.

MarkEWaite pushed a commit to jenkinsci/authorize-project-plugin that referenced this pull request Jun 14, 2024
Thanks to @basil for raising awareness of this - jenkinsci/jenkins#9148 (comment).

We're now using the JavaScript fetch() API for the builds widget which requires 
a polyfill for HtmlUnit. This polyfill is enabled by default as part of the Jenkins 
test harness createWebClient(), however in this case that method wasn't used 
as a couple of methods of WebClient needed to be overridden.

This MR enables the polyfill for that web client.
@MarkEWaite
Copy link
Contributor

opened merge requests for both plugins.

Requests have been merged and released for both plugins. Pull requests were:

BOM pull requests submitted by dependabot to include in a new release of the plugin bill of materials:

I plan to deliver a new release of the plugin bill of materials within the next 8 hours. A release was just delivered successfully, so I don't expect any issue with the new release.

@MarkEWaite
Copy link
Contributor

Plugin bill of materials https://github.com/jenkinsci/bom/releases/tag/3135.v6d6c1f6b_3572 includes those two plugin releases.

@hankzhao-zp
Copy link

I found that when displaying a very long http link, it looks a bit bad
image

@timja
Copy link
Member

timja commented Jul 10, 2024

I found that when displaying a very long http link, it looks a bit bad image

can you raise an issue on https://issues.jenkins.io please?

@hankzhao-zp
Copy link

I found that when displaying a very long http link, it looks a bit bad image

can you raise an issue on https://issues.jenkins.io please?

No problem, it's done
https://issues.jenkins.io/browse/JENKINS-73437

Comment on lines +31 to +32
// Avoid fetching if the page isn't active
if (document.hidden) {
Copy link
Member

Choose a reason for hiding this comment

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

I filed JENKINS-73613 for this, seems like poor UX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ath-successful This PR has successfully passed the full acceptance-test-harness suite pct-fail The plugin-compatibility-test suite needs a forward-compatible change 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 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.

9 participants