Skip to content
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

Respect RUNNER_TEMP environment variable #194

Closed

Conversation

svdschoot-wefabricate
Copy link

On some (self hosted) runners os.tmpdir() cannot be used to communicate between applications. For example when using snap, every snap will get it's own, isolated, os.tmpdir().

Github already provides us with an environment variable, RUNNER_TEMP, that should point to a directory that we can use as a temporary storage location that should be accessible by different applications on the same host. This change will use the directory pointed to by RUNNER_TEMP as base for tmpDir when the variable is defined and the directory can be created or already existed

Comment on lines +26 to +36
private static readonly _tmpDir = fs.mkdtempSync(path.join(Context._tryCreateDirctory(process.env.RUNNER_TEMP) || os.tmpdir(), 'docker-actions-toolkit-'));

private static _tryCreateDirctory(directoryPath: string | undefined): string | undefined {
if (directoryPath !== undefined) {
const createdPath = fs.mkdirSync(directoryPath, {recursive: true});
if (createdPath !== directoryPath) {
return undefined;
}
}
return directoryPath;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think _tryCreateDirctory is necessary, can just be:

Suggested change
private static readonly _tmpDir = fs.mkdtempSync(path.join(Context._tryCreateDirctory(process.env.RUNNER_TEMP) || os.tmpdir(), 'docker-actions-toolkit-'));
private static _tryCreateDirctory(directoryPath: string | undefined): string | undefined {
if (directoryPath !== undefined) {
const createdPath = fs.mkdirSync(directoryPath, {recursive: true});
if (createdPath !== directoryPath) {
return undefined;
}
}
return directoryPath;
}
private static readonly _tmpDir = fs.mkdtempSync(path.join(process.env.RUNNER_TEMP || os.tmpdir(), 'docker-actions-toolkit-'));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A directory at path RUNNER_TEMP may not exist, in which case fs.mkdtempSync(path.join(process.env.RUNNER_TEMP, 'docker-actions-toolkit-')) would throw an exception. Even though the provided string is a prefix, the directory in which the temporary directory would be created must already exist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh good point. This means the tests also need to point RUNNER_TEMP to an existing directory tough.
Will update the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means the tests also need to point RUNNER_TEMP to an existing directory tough.
Will update the PR.

This is already set in jest config:

RUNNER_TEMP: path.join(tmpDir, 'runner-temp'),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants