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-60866] Un-inline setup wizard root URL js #7619
[JENKINS-60866] Un-inline setup wizard root URL js #7619
Conversation
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.
Code review found no issues.
Interactive testing of the setup wizard root URL field found no issues. The enter key is ignored if it is pressed inside the field, just as it was ignored previously.
Does it need review from @jenkinsci/core-security-review ? |
I didn't introduce anything new and stick to what was already in place; hence I believe we don't necessarily need a security review. Although, Wadeck advised to use the needs-security-review label if changes involve Jelly or JS changes (as far as I'm aware of) on the mailing list. |
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.
Added a link to this PR to https://issues.jenkins.io/browse/JENKINS-60866 (generic task for CSP).
When it's just a move, usually you cannot really introduce a vulnerability. It does not require the security audit I would say.
Also keep in mind that if you are "just" moving code, the review is also very straightforward ;)
Thanks for checking, clarifying and linking the issue. I wasn't aware we already track this somewhere 👀 /label ready-for-merge This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback. |
See JENKINS-60866 for the issue that covers the process. This pull request does not resolve that issue, but it is progress on the goal of that issue.
Testing done
Testing interactively, I didn't encounter any regressions while walking through the setup wizard until the root URL page.
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).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).