-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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-72579] Adjust heap dump file name for compatibility with OpenJDK file suffix requirements #8881
Conversation
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
@@ -215,6 +215,8 @@ public void doIndex(StaplerResponse rsp) throws IOException { | |||
@WebMethod(name = "heapdump.hprof") | |||
public void doHeapDump(StaplerRequest req, StaplerResponse rsp) throws IOException, InterruptedException { | |||
owner.checkPermission(Jenkins.ADMINISTER); | |||
String propertyName = "jdk.management.heapdump.allowAnyFileSuffix"; |
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.
wouldn't it be easier to just change the suffix to .hprof
in line 179?
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.
ok, I changed it.
58817f8
to
0364622
Compare
Software testing is necessary to identify and fix any errors or bugs and to ensure the software functions as expected, providing a reliable and user-friendly experience. For this reason, the Testing Done section of the PR template is mandatory, not optional. Can you please confirm that you can start Jenkins with |
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 good. Should include a changelog entry I think.
@basil, should I add it, or will the core team add it? If I should add it, how do I do so? |
You can edit the PR description to fill in the "Proposed changelog entries" section. |
@jenkinsci/core-security-review It's been a while since this feature worked. During the intervening years, are there any new security requirements that would inhibit the reintroduction of this feature? |
@basil I have updated the changelog. |
This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks! |
Congratulations on getting your very first Jenkins core pull request merged 🎉🥳 |
See JENKINS-72579
Testing done
Done
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist