Header extension JEP #1
Conversation
Co-authored-by: Raúl Arabaolaza Barquin <raul.arabaolaza@gmail.com>
Co-authored-by: Raúl Arabaolaza Barquin <raul.arabaolaza@gmail.com>
Co-authored-by: Raúl Arabaolaza Barquin <raul.arabaolaza@gmail.com>
Co-authored-by: Francisco Javier Fernandez <31063239+fcojfernandez@users.noreply.github.com>
|
https://github.com/imonteroperez/jep/blob/header-extension-jep/jep/401/README.adoc for the currently rendered document |
There was a problem hiding this comment.
Proposing some changes, asking questions. Overall good proposal. Due to the size of the PR, the impact of the changes, I do not see the requirement of the JEP. Yes the detailed explanation is good, but does not feel necessary there.
The only "controversial" discussion that could happen is about "performance cost" from moving the Jelly to an extension lookup but this seems to be close to 0% impact.
Compared to the first approach with full nectarization, I have no concern to support this change.
No issues in terms of security as you are not loading "arbitrary" jelly files, but only the ones provided by the extension implementation. (that was my fear when you requested the security review)
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
Co-authored-by: Wadeck Follonier <Wadeck@users.noreply.github.com>
imonteroperez
force-pushed
the
header-extension-jep
branch
from
300826a to
b59850a
Compare
Remember when you do this, you block reviewers from being to able to see incremental changes. Unless you accidentally pushed nuclear launch codes ( |
| } | ||
| ``` | ||
|
|
||
| * Let’s consider the following implementation of the Jenkins header on: `core/src/main/java/jenkins/views/JenkinsHeader.java` |
There was a problem hiding this comment.
Just file a reference implementation PR and link to it. A JEP is not a good place for code review.
|
Closed in favor of: jenkinsci#380 and jenkinsci/jenkins#5909 |
No description provided.