-
-
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
Move AbstractBuild/index.jelly
to Run/index.jelly
#8110
Move AbstractBuild/index.jelly
to Run/index.jelly
#8110
Conversation
@jglick any issues with this? it sounds like a good idea to me |
@janfaracik can you file a downstream PR to workflow-job removing it's copy after checking to see if there's any differences? |
|
Therefore most of |
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.
Per above.
In this case it would make sense to split the index.jelly into a "frame" jelly page and multiple local parts, that can be implemented or overwritten for specific types. Would that be feasible? @janfaracik did you try to make a diff of both files? Are there other known implementations? |
Yeah. End goal would be a widget system where Jenkins/plugins can plug their own widgets into the page.
Yeah I went through and I believe the summary components were the only differences.
I need to have a look around. |
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 @daniel-beck said, basically. There are other things that do not align. For example the upstream/downstream builds section would not exist for WorkflowRun
.
If there are particular sections of the page that seem duplicated and are likely to change often, then introduce a new helper fragment for them, and consume that in workflow-job
.
<j:set var="set" value="${it.changeSet}" /> | ||
<t:summary icon="symbol-changes"> | ||
<j:choose> | ||
<j:when test="${it.hasChangeSetComputed()}"> | ||
<st:include it="${set}" page="digest.jelly" /> |
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 equivalent seems to be missing in the downstream PR.
I think the source of confusion here is that #2730 did not bother to update AbstractBuild/index.jelly
to use .changeSets
. If we had a mixin view fragment for RunWithSCM
then both AbstractBuild
and WorkflowRun
could use it. (It does not make sense for any Run
, specifically https://github.com/jenkinsci/external-monitor-job-plugin/blob/683c09d993b9e9da5f883f4d0696eb45af323b55/src/main/java/hudson/model/ExternalRun.java#L51 which would not implement this interface. Be sure to test against that plugin!)
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.
Let me know if I'm misunderstanding - So add a file that uses the updated changesets method, then <st:include ... />
it in both AbstractBuild/main.jelly
and WorkflowRun/main.jelly
?
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.
Yes something like that. Could be a file such as RunWithSCM/changeSets.jelly
. I do not recall details of how Stapler looks for view fragments, whether it check interfaces or only superclasses, so you would need to play a bit to find the simplest st:include
form.
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.
I've added <st:include page="changesets.jelly" class="jenkins.scm.RunWithSCM" />
to summary.jelly
in AbstractBuild
and WorkflowRun
. The implementation of changesets is different in the two files (as you noted above) with one difference being that WorkflowRun
lacks
<!-- dependency changes -->
<j:set var="depChanges" value="${it.getDependencyChanges(it.previousBuild)}"/>
<j:if test="${!empty(depChanges)}">
${%Changes in dependency}
<ol>
<j:forEach var="dep" items="${depChanges.values()}">
<li>
<a href="${rootURL}/${dep.project.url}" class="model-link">${dep.project.displayName}</a>
<st:nbsp/>
<j:choose>
<j:when test="${dep.from!=null}">
<a href="${rootURL}/${dep.from.url}" class="model-link inside">
<l:icon class="${dep.from.buildStatusIconClassName} icon-sm" alt="${dep.from.iconColor.description}"/>${dep.from.displayName}</a>
</j:when>
<j:otherwise>
?
</j:otherwise>
</j:choose>
→ <!-- right arrow -->
<a href="${rootURL}/${dep.to.url}" class="model-link inside">
<l:icon class="${dep.to.buildStatusIconClassName} icon-sm" alt="${dep.to.iconColor.description}" />${dep.to.displayName}</a>
(<a href="${rootURL}/${dep.project.url}changes?from=${dep.fromId}&to=${dep.toId}">${%detail}</a>)
</li>
</j:forEach>
</ol>
</j:if>
- Would this be needed in changesets.jelly
?
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.
I am not very familiar with this but looks like something never implemented in Pipeline.
In general, for a view refactoring like this it is necessary to examine every line in all variants, figure out what it is doing and where it came from (was this a copy of an old version of something that was since updated at the source?), and decide which parts can be made common without loss of functionality and which intrinsically need customization.
AbstractBuild/index.jelly
to Run/index.jelly
AbstractBuild/index.jelly
to Run/index.jelly
Please take a moment and address the merge conflicts of your pull request. Thanks! |
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.
Looks to be on the right track but there is at least one mistake here. There may be others but I have not gone through each line and tried to verify each one individually; unfortunately without JENKINS-31203 this is a manual process and is something the author should do. (It is nice to leave self-reviews with notes for reviewers.) Should be kept in draft status until the full set of changes including in jenkinsci/workflow-job-plugin#364 is (self-)reviewed and some sanity tests have been run on freestyle builds, Pipeline builds, external builds, and to be on the safe side maybe matrix builds (top-level & configuration) checking that UI elements mentioned in the diff are preserved.
Have you checked all the changes now @janfaracik ? |
These are the checks I've done. Had to make a couple of tweaks regarding AbstractBuild to maintain compatibility but all seems alright. |
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.
nice work great change
have not looked again recently
Please take a moment and address the merge conflicts of your pull request. Thanks! |
Any thoughts on moving this forward? |
Please take a moment and address the merge conflicts of your pull request. Thanks! |
@jenkinsci/core-pr-reviewers would someone mind taking a look please? |
/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! |
Caused JENKINS-73168. |
I have added an ATH workaround in jenkinsci/acceptance-test-harness#1545. jenkinsci/acceptance-test-harness#1548 tracks the removal of this workaround as part of fixing JENKINS-73168. |
Would be good to have some discussion over this.
This PR/discussion proposes moving the build page from
AbstractBuild
to its parentRun
, this is so that plugins that extendRun
can use the page rather than having to reimplement it (e.g. like workflow-job-plugin).It would be preferable to have plugins extend pages rather than to reimplement them to make changes in
core
easier and more consistent. This will be important with upcoming works on the Jenkins user experience, such as introducing app bars, new widgets and adjusting the sidebar.For AbstractBuild I've split out Changesets/Upstream and downstream relationships into separate files, one called
summary.jelly
for use in the summary list, the other intomain.jelly
.Searched for classes that extend
Run
with https://github.com/search?q=org%3Ajenkinsci+%22class%22+%22extends+Run%3C%22&type=codeFrom that search the following plugins extend
Run
:Happy to hear thoughts on the matter (and alternatives if I'm misunderstanding how everything works).
Testing done
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@jenkinsci/sig-ux
Maintainer checklist
Before the changes are marked as
ready-for-merge
:upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).