-
Notifications
You must be signed in to change notification settings - Fork 335
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-69656] Un-inlining AbstractTestResultAction/summary.jelly for CSP compliance #444
Conversation
…d refactoring the script content
// Retrieving the link from UI allowing to show all failed tests | ||
// Note: we are retrieving the link by its ID to match how it was already done | ||
// in the showFailures method above. | ||
const showFailuresLink = document.getElementById("showLink"); |
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.
Note:
The Jira issue (JENKINS-69656) mentioned:
Be careful, the id "showLink" is perhaps too generic (could be present on other elements in the same page. Adding a className like "showFailuresLink" and using it as selector could be a good idea.
But the original piece of code was actually using document.getElementById("showLink")
to identify the link, so I considered it safe to rely on the very same identification for adding the onclick
handler.
If you feel like it's not good enough, I'm happy to change it, but the current proposal matches the original behavior of the source code.
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.
Perhaps a matter of luck, but we have several occurrences of showLink
being used as id: https://github.com/search?l=XML&q=org%3Ajenkinsci+showLink&type=Code
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.
Do you want me to change it in that PR with something unique (both in the onclick
and in the showFailures
function)?
Or should I create another issue to fix it? (or ignore it?)
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.
Advice: do not change code when not necessary for such "move". That makes the review more complicated and could introduce "new" issues.
Adding "arrow functions", "let", "spread operator", could also add compatibility issue. I quickly checked, it seems OK with https://www.jenkins.io/doc/administration/requirements/web-browsers/.
Not manually tested with different browsers. OK in terms of moving code for CSP PoV.
👍 Thanks for the advice! I was not sure about changing moved code actually, so I'll be careful not doing it in future PRs. As per browser compability, I assumed it was OK using things which were there for years, and checked that it was in line with Level 2 of Jenkins web-browsers compatibility (as Level 3 is basically everything). But I'm happy reverting if that's an issue. I manually tested with Brave, Chromium, Firefox on Linux. |
Hello there 👋
Hacktoberfest PR here to solve JENKINS-69656, by un-inlining AbstractTestResultAction/summary.jelly, and then making a step forward towards CSP adoption in Jenkins.
I tested that PR by following the details shared in the Jira ticket, and ensured my contribution didn't break the
current behavior.
While extracting the inline script to a js file, I also did a little bit of refactoring to the JavaScript source code, I
hope that's OK. Let me know if you don't like that part and I'll revert to the previous content of the JavaScript part.
Thanks a lot for your review, let me know if there's anything missing I can take care of 😄
Have a great day!