[JEP-234] Customizable header

[imonteroperez]

See JEP-234

Proposed changelog entries

• New extension point Header as an interface that provides capabilities to render a specific header and a default implementation of that, named JenkinsHeader that is enabled by default.

Proposed upgrade guidelines

N/A

Submitter checklist

• (If applicable) 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

@MarkEWaite @jglick @fcojfernandez @raul-arabaolaza @EstherAF @Wadeck @timja @batmat

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 must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[timja]

Can you write a proper changelog entry rather than a developer guide please, our changelog isn't setup for something that complex.

This should be probably be documented on jenkins.io too?

[oleg-nenashev]

I do not think this is a correct approach for such a change. As @timja mentioned, this makes maintenance more complicated. And it is not an extension point in classic means, it is just an override logic. It might be helpful for some parties if they want to replace the header alltogether bit it complicates further maintenance. As long as there is no reference implementation justifying it, I am not ready to support such design.

[imonteroperez]

I do not think this is a correct approach for such a change. As @timja mentioned, this makes maintenance more complicated. And it is not an extension point in classic means, it is just an override logic. It might be helpful for some parties if they want to replace the header alltogether bit it complicates further maintenance. As long as there is no reference implementation justifying it, I am not ready to support such design.

@oleg-nenashev in JEP-234 there is a reference implementation. Not sure if this is enough or not, but as mentioned in JEP, current alternatives make no possible (or a hacky/hell/nightmare-frankenstein-thing) to have a customized header (not only in UI terms)

[timja]

I do not think this is a correct approach for such a change. As @timja mentioned, this makes maintenance more complicated. And it is not an extension point in classic means, it is just an override logic. It might be helpful for some parties if they want to replace the header alltogether bit it complicates further maintenance. As long as there is no reference implementation justifying it, I am not ready to support such design.

@oleg-nenashev in JEP-234 there is a reference implementation. Not sure if this is enough or not, but as mentioned in JEP, current alternatives make no possible (or a hacky/hell/nightmare-frankenstein-thing) to have a customized header (not only in UI terms)

This is exactly what I was concerned with:

https://github.com/imonteroperez/custom-header-plugin/blob/main/src/main/resources/org/jenkinsci/plugins/custom/header/CustomHeader/headerContent.jelly

a narrower API would be preferred I think

[imonteroperez]

I do not think this is a correct approach for such a change. As @timja mentioned, this makes maintenance more complicated. And it is not an extension point in classic means, it is just an override logic. It might be helpful for some parties if they want to replace the header alltogether bit it complicates further maintenance. As long as there is no reference implementation justifying it, I am not ready to support such design.

@oleg-nenashev in JEP-234 there is a reference implementation. Not sure if this is enough or not, but as mentioned in JEP, current alternatives make no possible (or a hacky/hell/nightmare-frankenstein-thing) to have a customized header (not only in UI terms)

This is exactly what I was concerned with:

https://github.com/imonteroperez/custom-header-plugin/blob/main/src/main/resources/org/jenkinsci/plugins/custom/header/CustomHeader/headerContent.jelly

a narrower API would be preferred I think

No strong opinion on to setup API endpoints for each part of the header content (logo, searchbar, etc.) to make it more modular, but not part of the scope of the JEP proposed here. JEP aims to have capabilities to include/update parts/all the header beyond styles and static content. I guess I get your point and I see that as a follow up of this in order to apply KISS :-)

[timja]

Again as Oleg and I have said this isn't extending this is replacing.

This does not explain backwards compatibility for Jenkins core in terms of the header:
https://github.com/imonteroperez/jep/blob/jep-401-header/jep/234/README.adoc#backwards-compatibility

If we refactor the header what happens to your custom header, could be completely broken.
If you want to continue down this path it needs to be clearly documented as a non stable API as this sort of API could never be stable imo.

we could change CSS and adapt the header but your custom one would not be adapted at all.

[imonteroperez]

Again as Oleg and I have said this isn't extending this is replacing.

I'm fine with that statement. I have not mentioned at all it is about extending. Actually, as you mentioned, the JEP proposal aims to make it feasible to customize by a replacement of the pageHeader.jelly that should look like:

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:i="jelly:fmt" xmlns:x="jelly:xml">
	<j:invokeStatic var="header" className="jenkins.views.JenkinsHeader" method="get"/>
	<st:include it="${header}" page="headerContent.jelly"/>
</j:jelly>

And each plugin that provides a Header implementation can replace the headerContent. The main benefit here is that this open the door to have capabilities in Jenkins to customize the header beyond UI and static content (like CSS), so we would be able to do things like:

<j:invokeStatic var="label" className="org.jenkinsci.plugins.custom.header.CustomHeader" method="getHeaderLabel"/>
<span class="hidden-xs hidden-sm">${label} - ${userName}</span>

where label can be something that is configured/updated dynamically through a field on Global Configuration (or whatever) - delegating to Java to programmatically retrieve things to be rendered in the header or to add new business logic there.

This does not explain backwards compatibility for Jenkins core in terms of the header: https://github.com/imonteroperez/jep/blob/jep-401-header/jep/234/README.adoc#backwards-compatibility

If we refactor the header what happens to your custom header, could be completely broken. If you want to continue down this path it needs to be clearly documented as a non stable API as this sort of API could never be stable imo.

we could change CSS and adapt the header but your custom one would not be adapted at all.

Well, I guess this is the same issue that could happen with other approaches like simple-theme-plugin. For example: if we decide tomorrow to modify the id of the search-box on the header: https://github.com/jenkinsci/jenkins/blob/master/core/src/main/resources/lib/layout/pageHeader.jelly#L53, this will probably affect a lot of themes using that customization tooling (i.e: https://github.com/djonsson/jenkins-atlassian-theme/blob/master/src/style.css#L95)

IOW (and setting expectations) as mentioned before I'm ok with having a narrowed API that could help here to fix the mentioned issues, but that was not the proposal that is on the JEP. The proposal of the JEP aims to have capabilities to include/update parts/all the header beyond styles and static content.

[batmat]

I do not think this is a correct approach for such a change. As @timja mentioned, this makes maintenance more complicated

@oleg-nenashev I think your feedback has been addressed. Also given Tim approved this PR in the meantime, care to re-review?
Thanks!

---

Apart from the comment above, it looks like we're close to ready for merge. I think it would be cleaner however to officially merge the JEP PR before merging the core PR here.

@imonteroperez there has been comments by Jesse & Mark on the JEP, can you please make sure they're all addressed and marked as resolved so we can move towards merging there and here? Thanks for the great work!

[imonteroperez]

@imonteroperez there has been comments by Jesse & Mark on the JEP, can you please make sure they're all addressed and marked as resolved so we can move towards merging there and here? Thanks for the great work!

@batmat all comments addressed and marked as resolved. Not able to merge the JEP PR (restricted to authorized users)

[batmat]

@oleg-nenashev I think your feedback was addressed, so I'm dismissing your past review FYI. Feel free to tell us whether you still have comments or requests on the current implementation.

Stale and deemed addressed.

[batmat]

Given the 6 approvals, I am marking this PR ready-for-merge.

However, I'd like to condition the merge here to be after jenkinsci/jep#380 has been merged as Draft JEP and assigned an official JEP number.

In other words, this means I plan to merge here 24 hours from now, provided the JEP PR has been merged in the meantime.

Please tell us if you disagree with this stance. Thanks all.

[oleg-nenashev]

Sorry @batmat but I am against merging in the current state. I would like to review it, and AFAICT there was no review requested from me. Sorry, I missed the previous comments but we expect to request reviews in this repository.

The Tuesday weekly ship has sailed, so there is no problem with keeping the pull request open until the JEP is properly integrated. I will handle that and do the review

[imonteroperez]

Sorry @batmat but I am against merging in the current state. I would like to review it, and AFAICT there was no review requested from me. Sorry, I missed the previous comments but we expect to request reviews in this repository.

It was requested 12 days ago, see: #5909 (comment)

[oleg-nenashev]

I might be missing something. As far as I can tell it wasn't on my review dashboard. If it was requested through the comment, I can assure you I missed that.

Back to the subject matter, would it be possible to summarize the current review state in the developer mailing list thread? It would be particularly useful to know whether it was presented to the user experience group like it was announced there, and what is the consensus built in the UX SIG.

For the record I do not intend to block the change further. What I wanted is to ensure that we follow the JEP process as much as possible since it was taken for this pull request. We have one week before the next weekly, so it makes no sense to rush now and to risk creating leftovers

I will follow up on my proposal to add all officers and board members to JEP editors. Apparently even the recent changes are not enough

[imonteroperez]

Back to the subject matter, would it be possible to summarize the current review state in the developer mailing list thread? It would be particularly useful to know whether it was presented to the user experience group like it was announced there, and what is the consensus built in the UX SIG.

I will summarize all in the dev ML thread tomorrow after the UX SIG meeting

[batmat]

For the record, the associated JEP for this change here is now officially JEP-234.

[imonteroperez]

Back to the subject matter, would it be possible to summarize the current review state in the developer mailing list thread? It would be particularly useful to know whether it was presented to the user experience group like it was announced there, and what is the consensus built in the UX SIG.

I will summarize all in the dev ML thread tomorrow after the UX SIG meeting

FTR I shared JEP-234 in the Jenkins UX SIG meeting. Meeting notes are available here

Also shared with them the implementation details from this PR

Feedback obtained from the group was positive and they did not see any issue with this approach. 

Shared this also on the jenkins-dev ML: https://groups.google.com/g/jenkinsci-dev/c/1tDvSioCaF0

cc/ @oleg-nenashev

[imonteroperez]

Any pending action here before merging this PR? @oleg-nenashev @batmat

[timja]

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

addressed

[imonteroperez]

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@timja Can we merge this? No negative feedback was obtained in 24h.

[timja]

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.
Thanks!

@timja Can we merge this? No negative feedback was obtained in 24h.

generally we batch merge before weekly 😄 so no need to worry if its more than 24h