-
Notifications
You must be signed in to change notification settings - Fork 143
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-60082] Show pull request title in name column #476
Conversation
NB: |
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.
Seems reasonable, javadoc on ItemColumn.getTitle()
will be incorrect with this change.
I wonder how this scales with longer titles though.
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.
Going to leave open for a period to let others comment that may recall any history better than me.
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.
</j:when> | ||
<j:otherwise> | ||
${it.getTitle(job)} | ||
<span style="opacity: 0.75; margin-left: 0.625rem; white-space: nowrap;"><l:breakable value="${h.getRelativeDisplayNameFrom(job, itemGroup)}"/></span> |
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.
What about adding brackets around the PR branch to more clearly separate?
<span style="opacity: 0.75; margin-left: 0.625rem; white-space: nowrap;"><l:breakable value="${h.getRelativeDisplayNameFrom(job, itemGroup)}"/></span> | |
<span style="opacity: 0.75; margin-left: 0.625rem; white-space: nowrap;"><l:breakable value="(${h.getRelativeDisplayNameFrom(job, itemGroup)})"/></span> |
I added a screenshot to description from my testing (also shows dark mode) |
Browsed around a bit, tried setting
Haven't played too much but it appears there's a trait that can be supplied to set the display name to the PR title - maybe it'd make sense to provide a default trait or something like that. |
Right just change this
FWIW the traits don't look the best right now and could do with an improvement:
Some have opacity lowered but not all for some reason
|
perhaps this is all wrong (I forgot about the traits). |
I think a default trait should be added and most of this can likely be reverted in the jelly at least
No I don't think we want columns, sensible display names by default is what is requested here.
Separate to this but sure. |
Whats with all the test failures? |
// The raw name provided here in the context of pull requests is the pull request ID | ||
// We tidy up the ID so that they display consistently between SCMs | ||
String cleanedUpBranchName = rawName; | ||
if (cleanedUpBranchName.startsWith("MR-") || cleanedUpBranchName.startsWith("PR-")) { |
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.
may need a regex that it's PR-\d+ so its less likely to hit an actual branch name
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.
a branch name won't be hit here because the rawName
and displayName
would be the same so the above isn't a concern.
Failing on |
Typically means that tests are not cleaning up after themselves, and are interfering with one another for example by trying to use the same job name.
|
<td style="${indenter.getCss(job)}"> | ||
<d:taglib uri="local"> | ||
<d:tag name="link"> | ||
<a href="${jobBaseUrl}${job.shortUrl}" class='model-link jenkins-table__link'> |
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.
extracts the same code into one place and makes minor changes to the classes
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.
Why inside
removal though? It's present in https://github.com/jenkinsci/jenkins/blob/e1f99468fd81d6aa52dc434fefc6e0420e1cb449/core/src/main/resources/hudson/views/JobColumn/column.jelly#L29
try (BulkChange bc = new BulkChange(project);) { | ||
observer.created(project); | ||
project.setDisplayName(getProjectDisplayName(project, rawName)); | ||
bc.commit(); | ||
} catch (IOException e) { | ||
// Ignored | ||
} |
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.
this change fixed the tests. the observer.created is what creates the RunMap
that was null before. (noticed by walking back through a stacktrace of a successful one to see where it was created).
I'm not 100% sure on if there's any impact here and why it was so keen to avoid saving / why a bulk change couldn't be used around everything...
Personal preference for me woul'd be to have the cr/pr id first then the title, e.g. |
* @return the tool-tip title unescaped for use in an attribute. | ||
*/ | ||
@SuppressWarnings("unused") // used via Jelly EL binding | ||
public String getTitle(Object job) { |
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.
Removing an old public method signature.
@jtnord could you take another look please? |
So for display purposes I would agree with Tim, but for sorting purposes I would want them sorted by PR number (which is also what GH does!). (this is why I originally thought it would be better as 2 columns because I am not sure that the conflicting use cases can be solved here). |
// The raw name provided here in the context of pull requests is the pull request ID | ||
// We tidy up the ID so that they display consistently between SCMs | ||
String cleanedUpBranchName = rawName; | ||
if (cleanedUpBranchName.startsWith("MR-") || cleanedUpBranchName.startsWith("PR-")) { |
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.
this would appear to be putting SCM specifics into an SCM agnostic piece of code?
I believe other systems exist and Gerrit uses C-xxxx
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.
Is this a blocker, and if so would this mean an API likely needs to be created where the SCMs set their prefix?
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.
So initially I was not thinking it was a blocker, but something that should be addressed (in the future as a follow up)
But now I am thinking is this just confusing? we use #nnn
for build numbers in Jenkins and do I really want or need consistency across SCMs? Indeed having PR-123 in one job and MR-245 in another is a reminder that one project is in my GitHub.com account and the other is in GitLab on premise.
in other words could a user expect that by clicking that link below they get taken to build #7 of the job?
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.
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.
the PR-xx -> #xx can always be reverted/changed/made robust with a new API in the future if there is any feedback in this area.
🚢 ? |
sorry, thought you where a maintainer here as you had a green tick for the review, but forgot that was from the access you have on Jenkins as a whole :) |
<j:choose> | ||
<j:when test="${it.isOrphaned(job)}"> | ||
<s> | ||
<a href="${jobBaseUrl}${job.shortUrl}" class='model-link inside jenkins-table_link' title="${it.getTitle(job)}"> |
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.
@janfaracik Why did you remove the inside
class?
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.
It doesn't do anything from what I've seen from core.
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.
It controlled whether the context menu dropdown chevron was considered part of the area of the model-link
. If it doesn't do anything anymore, that's an oversight likely in jenkinsci/jenkins#6084.
2.332.x otherwise (ugly but this hover effect was fairly short-lived):
It seems this caused a long title of a pull request to be line-wrapped in a strange way, in Mozilla Firefox 128.3.1esr. Redacted by changing most letters to A and most digits to 1: <tr id="job_PR-1111" class=" job-status-blue"><td data="4" class="jenkins-table__cell--tight jenkins-table__icon"><div class="jenkins-table__cell__button-wrapper"><span style="width: 24px; height: 24px; " class="build-status-icon__wrapper icon-blue icon-md"><span class="build-status-icon__outer"><svg viewBox="0 0 24 24" tooltip="Success" focusable="false" class="svg-icon " title="Success"><use href="/images/build-status/build-status-sprite.svg#build-status-static"></use></svg></span><svg viewBox="0 0 24 24" tooltip="Success" focusable="false" class="svg-icon icon-blue icon-md" title="Success"><use href="/static/bac444bb/images/build-status/build-status-sprite.svg#last-successful"></use></svg></span></div></td><td data="100" class="jenkins-table__cell--tight jenkins-table__icon healthReport" data-html-tooltip="<div class="jenkins-tooltip--table-wrapper"><table class="jenkins-table"><thead><tr><th class="jenkins-!-padding-left-0" align="center">W</th><th align="left">Description</th><th align="right">%</th></tr></thead><tbody><tr><td align="left" class="jenkins-table__cell--tight jenkins-table__icon"><div class="jenkins-table__cell__button-wrapper"><svg xmlns="http://www.w3.org/2000/svg" aria-hidden="true" height="512" viewBox="0 0 512 512" width="512"><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="256" x2="256" y1="48" y2="96"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="256" x2="256" y1="416" y2="464"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="403.08" x2="369.14" y1="108.92" y2="142.86"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="142.86" x2="108.92" y1="369.14" y2="403.08"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="464" x2="416" y1="256" y2="256"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="96" x2="48" y1="256" y2="256"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="403.08" x2="369.14" y1="403.08" y2="369.14"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="142.86" x2="108.92" y1="142.86" y2="108.92"/><circle cx="256" cy="256" r="80" style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px"/></svg></div></td><td align="left">Test Result: 0 tests failing out of a total of 256 tests.</td><td align="right">100</td></tr><tr><td align="left" class="jenkins-table__cell--tight jenkins-table__icon"><div class="jenkins-table__cell__button-wrapper"><svg xmlns="http://www.w3.org/2000/svg" aria-hidden="true" height="512" viewBox="0 0 512 512" width="512"><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="256" x2="256" y1="48" y2="96"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="256" x2="256" y1="416" y2="464"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="403.08" x2="369.14" y1="108.92" y2="142.86"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="142.86" x2="108.92" y1="369.14" y2="403.08"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="464" x2="416" y1="256" y2="256"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="96" x2="48" y1="256" y2="256"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="403.08" x2="369.14" y1="403.08" y2="369.14"/><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="142.86" x2="108.92" y1="142.86" y2="108.92"/><circle cx="256" cy="256" r="80" style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px"/></svg></div></td><td align="left">Build stability: No recent builds failed.</td><td align="right">100</td></tr></tbody></table></div>"><div class="jenkins-table__cell__button-wrapper"><a href="job/PR-1111/lastBuild" class="build-health-link jenkins-button jenkins-button--tertiary"><svg xmlns="http://www.w3.org/2000/svg" aria-hidden="true" height="512" viewBox="0 0 512 512" width="512"><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="256" x2="256" y1="48" y2="96"></line><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="256" x2="256" y1="416" y2="464"></line><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="403.08" x2="369.14" y1="108.92" y2="142.86"></line><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="142.86" x2="108.92" y1="369.14" y2="403.08"></line><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="464" x2="416" y1="256" y2="256"></line><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="96" x2="48" y1="256" y2="256"></line><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="403.08" x2="369.14" y1="403.08" y2="369.14"></line><line style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px" x1="142.86" x2="108.92" y1="142.86" y2="108.92"></line><circle cx="256" cy="256" r="80" style="fill:none;stroke:var(--yellow);stroke-linecap:round;stroke-miterlimit:10;stroke-width:36px"></circle></svg></a></div></td><td><a href="job/PR-1111/" class="model-link jenkins-table__link">AAAAA<wbr>-1111, Aaaaaaaaa AaaaaAaaAaaaaaaaaAaa<wbr>aaAaaaaaaa aaaaa aaa AAA aaaaaaaaaaa aaaaaaaa aa aaaaaaa aaaaa aaaaa <wbr>(#1111)<span class="jenkins-menu-dropdown-chevron" data-href="https://jenkins.example/job/AAAA/job/Aaaaa-Aaaaaa/view/change-requests/job/PR-1111/" aria-expanded="false"></span></a></td><td data="2024-10-28T11:16:15Z">
22 hr
<a href="job/PR-1111/lastSuccessfulBuild/" class="jenkins-table__link jenkins-table__badge model-link inside">#1<span class="jenkins-menu-dropdown-chevron" data-href="https://jenkins.example/job/AAAA/job/Aaaaa-Aaaaaa/view/change-requests/job/PR-1111/lastSuccessfulBuild/" aria-expanded="false"></span></a></td><td data="-">N/A</td><td data="767903">12 min</td><td class="jenkins-table__cell--tight"><div class="jenkins-table__cell__button-wrapper"><a tooltip="Schedule a Build with parameters for AAAAA-1111, Aaaaaaaaa AaaaaAaaAaaaaaaaaAaaaaAaaaaaaa aaaaa aaa AAA aaaaaaaaaaa aaaaaaaa aa aaaaaaa aaaaa aaaaa (#1111)" id="id756" href="job/PR-1111/build?delay=0sec" class="jenkins-button jenkins-button--tertiary jenkins-!-build-color " title="Schedule a Build with parameters for AAAAA-1111, Aaaaaaaaa AaaaaAaaAaaaaaaaaAaaaaAaaaaaaa aaaaa aaa AAA aaaaaaaaaaa aaaaaaaa aa aaaaaaa aaaaa aaaaa (#1111)"><svg xmlns="http://www.w3.org/2000/svg" aria-hidden="true" height="512" viewBox="0 0 512 512" width="512"><title></title><path d="M112,111V401c0,17.44,17,28.52,31,20.16l247.9-148.37c12.12-7.25,12.12-26.33,0-33.58L143,90.84C129,82.48,112,93.56,112,111Z" style="fill:none;stroke:currentColor;stroke-miterlimit:10;stroke-width:32px"></path></svg></a></div></td><td align="right">n/a</td><td data="64" data-html-tooltip="<div class="jenkins-tooltip healthReportDetails jenkins-tooltip--table-wrapper"><table class="jenkins-table"><thead><tr><th></th><th align="left">Tool</th><th align="right">Total</th></tr></thead><tbody><tr><td align="left"><svg class="icon-md" xmlns="http://www.w3.org/2000/svg" aria-hidden="true" viewBox="0 0 512 512"><!--! Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL 1.1, Code: MIT License) Copyright 2024 Fonticons, Inc. --><path d="M256 32c14.2 0 27.3 7.5 34.5 19.8l216 368c7.3 12.4 7.3 27.7 .2 40.1S486.3 480 472 480L40 480c-14.3 0-27.6-7.7-34.7-20.1s-7-27.8 .2-40.1l216-368C228.7 39.5 241.8 32 256 32zm0 128c-13.3 0-24 10.7-24 24l0 112c0 13.3 10.7 24 24 24s24-10.7 24-24l0-112c0-13.3-10.7-24-24-24zm32 224a32 32 0 1 0 -64 0 32 32 0 1 0 64 0z" fill="currentColor"/></svg></td><td><a href="/job/AAAA/job/Aaaaa-Aaaaaa/view/change-requests/job/PR-1111//1/msbuild">MSBuild (Build) Warnings</a></td><td align="right">64</td></tr><tr><td align="left"><svg class="icon-md" xmlns="http://www.w3.org/2000/svg" aria-hidden="true" viewBox="0 0 512 512"><!--! Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL 1.1, Code: MIT License) Copyright 2024 Fonticons, Inc. --><path d="M256 32c14.2 0 27.3 7.5 34.5 19.8l216 368c7.3 12.4 7.3 27.7 .2 40.1S486.3 480 472 480L40 480c-14.3 0-27.6-7.7-34.7-20.1s-7-27.8 .2-40.1l216-368C228.7 39.5 241.8 32 256 32zm0 128c-13.3 0-24 10.7-24 24l0 112c0 13.3 10.7 24 24 24s24-10.7 24-24l0-112c0-13.3-10.7-24-24-24zm32 224a32 32 0 1 0 -64 0 32 32 0 1 0 64 0z" fill="currentColor"/></svg></td><td><a href="/job/AAAA/job/Aaaaa-Aaaaaa/view/change-requests/job/PR-1111//1/msbuild-archive">MSBuild (Archive) Warnings</a></td><td align="right">0</td></tr></tbody></table></div>" class="healthReport issues-total" data-tooltip-interactive="true">64<div class="jenkins-tooltip healthReportDetails jenkins-tooltip--table-wrapper"><table class="jenkins-table"><thead><tr><th></th><th align="left">Tool</th><th align="right">Total</th></tr></thead><tbody><tr><td align="left"><svg class="icon-md" xmlns="http://www.w3.org/2000/svg" aria-hidden="true" viewBox="0 0 512 512"><!--! Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL 1.1, Code: MIT License) Copyright 2024 Fonticons, Inc. --><path d="M256 32c14.2 0 27.3 7.5 34.5 19.8l216 368c7.3 12.4 7.3 27.7 .2 40.1S486.3 480 472 480L40 480c-14.3 0-27.6-7.7-34.7-20.1s-7-27.8 .2-40.1l216-368C228.7 39.5 241.8 32 256 32zm0 128c-13.3 0-24 10.7-24 24l0 112c0 13.3 10.7 24 24 24s24-10.7 24-24l0-112c0-13.3-10.7-24-24-24zm32 224a32 32 0 1 0 -64 0 32 32 0 1 0 64 0z" fill="currentColor"></path></svg></td><td><a href="/job/AAAA/job/Aaaaa-Aaaaa/view/change-requests/job/PR-1111//1/msbuild">MSBuild (Build) Warnings</a></td><td align="right">64</td></tr><tr><td align="left"><svg class="icon-md" xmlns="http://www.w3.org/2000/svg" aria-hidden="true" viewBox="0 0 512 512"><!--! Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL 1.1, Code: MIT License) Copyright 2024 Fonticons, Inc. --><path d="M256 32c14.2 0 27.3 7.5 34.5 19.8l216 368c7.3 12.4 7.3 27.7 .2 40.1S486.3 480 472 480L40 480c-14.3 0-27.6-7.7-34.7-20.1s-7-27.8 .2-40.1l216-368C228.7 39.5 241.8 32 256 32zm0 128c-13.3 0-24 10.7-24 24l0 112c0 13.3 10.7 24 24 24s24-10.7 24-24l0-112c0-13.3-10.7-24-24-24zm32 224a32 32 0 1 0 -64 0 32 32 0 1 0 64 0z" fill="currentColor"></path></svg></td><td><a href="/job/AAAA/job/Aaaaa-Aaaaaa/view/change-requests/job/PR-1111//1/msbuild-archive">MSBuild (Archive) Warnings</a></td><td align="right">0</td></tr></tbody></table></div></td></tr> The same HTML renders OK in Microsoft Edge 130.0.2849.56, though: Adding a <td><a href="job/PR-1111/" class="model-link jenkins-table__link"><span>AAAAA<wbr>-1111, Aaaaaaaaa AaaaaAaaAaaaaaaaaAaa<wbr>aaAaaaaaaa aaaaa aaa AAA aaaaaaaaaaa aaaaaaaa aa aaaaaaa aaaaa aaaaa <wbr>(#1111)</span><span class="jenkins-menu-dropdown-chevron" data-href="https://jenkins.example/job/AAAA/job/Aaaaa-Aaaaaa/view/change-requests/job/PR-1111/" aria-expanded="false"></span></a></td> |
I also tried restoring the |
See #492 for the Firefox fix |
FYI JENKINS-74811 |
See JENKINS-60082
Would be good to get some thoughts on this/if this is the right approach.
Currently doing a survey on what users find useful on the Jenkins Dashboard (https://forms.gle/6XgzRshaF34kn4wLA) and quite a few of the responses list being able to show the pull request title on the dashboard as being useful (and I agree). So thought I'd take a look into that.
The plugin currently displays the pull request title as the
title
attribute of the link, meaning users have to hover over the PR to reveal what it is. This PR changes that so that it displays the title inline.Branch job dashboard:
Pull requests dashboard:
Project status page:
Testing done
Multi branch pipelines from before this change will have the old display name but if they are scanned they will get the new display name
Submitter checklist