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

Cache on ghes #363

Merged
merged 14 commits into from Mar 31, 2022
2 changes: 1 addition & 1 deletion .licenses/npm/@actions/cache.dep.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 42 additions & 1 deletion __tests__/utils.test.ts
@@ -1,8 +1,12 @@
import * as cache from '@actions/cache';
import {
validateVersion,
validatePythonVersionFormatForPyPy
validatePythonVersionFormatForPyPy,
isCacheFeatureAvailable
} from '../src/utils';

jest.mock('@actions/cache');

describe('validatePythonVersionFormatForPyPy', () => {
it.each([
['3.6', true],
Expand Down Expand Up @@ -32,3 +36,40 @@ describe('validateVersion', () => {
expect(validateVersion(input)).toEqual(expected);
});
});

describe('isCacheFeatureAvailable', () => {
it('isCacheFeatureAvailable disabled on GHES', () => {
jest.spyOn(cache, 'isFeatureAvailable').mockImplementation(() => false);
try {
process.env['GITHUB_SERVER_URL'] = 'http://example.com';
isCacheFeatureAvailable();
} catch (error) {
expect(error).toHaveProperty(
'message',
'Caching is only supported on GHES version >= 3.5. If you are on version >=3.5 Please check with GHES admin if Actions cache service is enabled or not.'
);
} finally {
delete process.env['GITHUB_SERVER_URL'];
}
});

it('isCacheFeatureAvailable disabled on dotcom', () => {
jest.spyOn(cache, 'isFeatureAvailable').mockImplementation(() => false);
try {
process.env['GITHUB_SERVER_URL'] = 'http://github.com';
isCacheFeatureAvailable();
} catch (error) {
expect(error).toHaveProperty(
'message',
'An internal error has occurred in cache backend. Please check https://www.githubstatus.com/ for any ongoing issue in actions.'
);
} finally {
delete process.env['GITHUB_SERVER_URL'];
}
});

it('isCacheFeatureAvailable is enabled', () => {
jest.spyOn(cache, 'isFeatureAvailable').mockImplementation(() => true);
expect(isCacheFeatureAvailable()).toBe(true);
});
});
17 changes: 12 additions & 5 deletions dist/cache-save/index.js
Expand Up @@ -3728,10 +3728,7 @@ const options_1 = __webpack_require__(538);
const requestUtils_1 = __webpack_require__(899);
const versionSalt = '1.0';
function getCacheApiUrl(resource) {
// Ideally we just use ACTIONS_CACHE_URL
const baseUrl = (process.env['ACTIONS_CACHE_URL'] ||
process.env['ACTIONS_RUNTIME_URL'] ||
'').replace('pipelines', 'artifactcache');
const baseUrl = process.env['ACTIONS_CACHE_URL'] || '';
if (!baseUrl) {
throw new Error('Cache Service Url not found, unable to restore cache.');
}
Expand Down Expand Up @@ -5920,7 +5917,8 @@ function downloadCacheStorageSDK(archiveLocation, archivePath, options) {
//
// If the file exceeds the buffer maximum length (~1 GB on 32-bit systems and ~2 GB
// on 64-bit systems), split the download into multiple segments
const maxSegmentSize = buffer.constants.MAX_LENGTH;
// ~2 GB = 2147483647, beyond this, we start getting out of range error. So, capping it accordingly.
const maxSegmentSize = Math.min(2147483647, buffer.constants.MAX_LENGTH);
const downloadProgress = new DownloadProgress(contentLength);
const fd = fs.openSync(archivePath, 'w');
try {
Expand Down Expand Up @@ -41451,6 +41449,15 @@ function checkKey(key) {
throw new ValidationError(`Key Validation Error: ${key} cannot contain commas.`);
}
}
/**
* isFeatureAvailable to check the presence of Actions cache service
*
* @returns boolean return true if Actions cache service feature is available, otherwise false
*/
function isFeatureAvailable() {
return !!process.env['ACTIONS_CACHE_URL'];
}
exports.isFeatureAvailable = isFeatureAvailable;
/**
* Restores cache from keys
*
Expand Down
3,531 changes: 1,774 additions & 1,757 deletions dist/setup/index.js

Large diffs are not rendered by default.

26 changes: 13 additions & 13 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
@@ -1,6 +1,6 @@
{
"name": "setup-python",
"version": "2.2.2",
"version": "2.2.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please change it to the planning version of the setup-python action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmitry-shibanov, Sorry I am not aware of the planned version. Could please share what is planned version.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be 3.0.1 or 3.1.0 to be consistent with the setup-python action releases: https://github.com/actions/setup-python/releases/tag/v3.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, even I found this to be wired that package.json has 2.2.2 and released tag 3.0.0 which we generally keep the same. So I thought this is already followed to keep them not in sync. Shall I fix this?

"private": true,
"description": "Setup python action",
"main": "dist/index.js",
Expand All @@ -23,7 +23,7 @@
"author": "GitHub",
"license": "MIT",
"dependencies": {
"@actions/cache": "^1.0.8",
"@actions/cache": "^2.0.0",
"@actions/core": "^1.2.3",
"@actions/exec": "^1.1.0",
"@actions/glob": "^0.2.0",
Expand Down
7 changes: 2 additions & 5 deletions src/setup-python.ts
Expand Up @@ -4,16 +4,13 @@ import * as finderPyPy from './find-pypy';
import * as path from 'path';
import * as os from 'os';
import {getCacheDistributor} from './cache-distributions/cache-factory';
import {isGhes} from './utils';
import {isCacheFeatureAvailable} from './utils';

function isPyPyVersion(versionSpec: string) {
return versionSpec.startsWith('pypy-');
}

async function cacheDependencies(cache: string, pythonVersion: string) {
if (isGhes()) {
throw new Error('Caching is not supported on GHES');
}
const cacheDependencyPath =
core.getInput('cache-dependency-path') || undefined;
const cacheDistributor = getCacheDistributor(
Expand Down Expand Up @@ -43,7 +40,7 @@ async function run() {
}

const cache = core.getInput('cache');
if (cache) {
if (cache && isCacheFeatureAvailable()) {
await cacheDependencies(cache, pythonVersion);
}
}
Expand Down
17 changes: 17 additions & 0 deletions src/utils.ts
@@ -1,3 +1,4 @@
import * as cache from '@actions/cache';
import fs from 'fs';
import * as path from 'path';
import * as semver from 'semver';
Expand Down Expand Up @@ -99,3 +100,19 @@ export function isGhes(): boolean {
);
return ghUrl.hostname.toUpperCase() !== 'GITHUB.COM';
}

export function isCacheFeatureAvailable(): boolean {
if (!cache.isFeatureAvailable()) {
if (isGhes()) {
throw new Error(
'Caching is only supported on GHES version >= 3.5. If you are on version >=3.5 Please check with GHES admin if Actions cache service is enabled or not.'
);
tiwarishub marked this conversation as resolved.
Show resolved Hide resolved
} else {
throw new Error(
'An internal error has occurred in cache backend. Please check https://www.githubstatus.com/ for any ongoing issue in actions.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this message accurate? It looks like cache.isFeatureAvailable() just looks for whether an environment variable is set.

https://github.com/actions/toolkit/blob/b4639928698a6bfe1c4bdae4b2bfdad1cb75016d/packages/cache/src/cache.ts#L53

Copy link
Contributor

Choose a reason for hiding this comment

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

If my understanding is correct, I would say something like

Suggested change
'An internal error has occurred in cache backend. Please check https://www.githubstatus.com/ for any ongoing issue in actions.'
'This runner is not configured to use the cache service. Caching will be skipped.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this env var is only set when cache service is present otherwise this var will not be set. This else condition is for dotcom scenario so ideally this scenario should never occur because AC is always there in dotcom. But just to cover corner scenario i have added this

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I understand better now https://github.com/actions/runner/blob/408d6c579c36f0eb318acfdafdcbafc872696501/src/Runner.Worker/Handlers/NodeScriptActionHandler.cs

How about:

Suggested change
'An internal error has occurred in cache backend. Please check https://www.githubstatus.com/ for any ongoing issue in actions.'
'The runner was not able to contact the cache service. Caching will be skipped.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Let me update the PR with changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: we don't want an issue with the cache service to block otherwise successful runs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Despite the conversation on the case above, I think this still stands as a place where we should warn instead of fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. I will update the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR

}
}

return true;
}