[JENKINS-16005] Use system temp directory for CommandInstaller scripts#26240
[JENKINS-16005] Use system temp directory for CommandInstaller scripts#26240meetgoti07 wants to merge 1 commit intojenkinsci:masterfrom
Conversation
The JENKINS-16021 issue is unrelated to this change. What is the issue that you are trying to resolve? |
|
@MarkEWaite Sorry for the confusion, I accidently mentioned Legacy Jira Issue number. |
1c47992 to
ce02890
Compare
ce02890 to
71211c2
Compare
|
Thanks for the pull request and thanks especially for outlining how you tested interactively. That is exactly the type of pull request description that we need. I can duplicate the test failure that is reported on ci.jenkins.io. I run the command:
I've made the pull request a draft pull request so that you can identify the cause of the test failure and either fix the test or make a change to the production code that both passes the automated test and passes your interactive tests. Once you've done that, please mark the pull request as "Ready for Review". |
|
@MarkEWaite |
AbstractCommandInstaller.performInstallation() was calling createTextTempFile() with the default inThisDirectory=true, causing it to try creating temp script files in the tool's installation directory (e.g., /usr). This fails when Jenkins doesn't have write access to that directory. Changed to use createTextTempFile(..., false) to create temp files in the system temp directory (java.io.tmpdir) instead.
fcd628a to
d837343
Compare
|
@meetgoti07 it is generally best for maintainers and other reviewers if you do not force push changes. When you force push a change, it breaks the connection between earlier comments and their matching commits. In this case, the force push did no harm because there were no comments on prior commits. However, as you continue to contribute, you'll find it best to leave the original commit and push a new commit. You can always label the pull request as "squash-merge-me" to assure that the incremental commits do not appear in the final history. |
|
@MarkEWaite Got it, thank you for the guidance. I will avoid force pushing and use incremental commits going forward. I have also fixed the test failure issue. |
Fixes #13136
AbstractCommandInstaller.performInstallation()was callingcreateTextTempFile()with the defaultinThisDirectory=true, causing it to create temp script files in the tool's installation directory (e.g.,/usr). This fails when Jenkins doesn't have write access to that directory.Changed to use
createTextTempFile(..., false)to create temp files in the system temp directory (java.io.tmpdir) instead.Added explicit dir.mkdirs() call to ensure the tool installation directory exists before running the script.
Testing done
Manually tested by:
/usr(a read-only directory on macOS)Failed to create a temp file on /usr/var/folders/.../T/hudsonXXX.sh)Screenshots
Before
After
Proposed changelog entries
Proposed changelog category
/label bug
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@daniel-beck @krisstern
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered.