Adding support in cache package to check if Artifact Cache service is enabled or not #1028
Conversation
|
LGTM |
packages/cache/src/cache.ts
Outdated
| */ | ||
|
|
||
| export function isFeatureAvailable(): boolean { | ||
| return utils.isFeatureAvailable() |
There was a problem hiding this comment.
Is it useful to put the function in utils instead of just inlining it here?
There was a problem hiding this comment.
In the toolkit repo, the unit test cases for the cache.ts file are not in a single file and they are distributed as per functionality like saveCachetest.ts and restoreCachetest.ts. And I also wanted to add a unit test for this function and so the only place I thought where it suits most was utils. Let me know if it make sense
There was a problem hiding this comment.
Personally I would opt for keeping the app code simple and pushing complexity into the test code, but it's not a big deal.
You can also simplify the current code like
export const isFeatureAvailable = utils.isFeatureAvailableThere was a problem hiding this comment.
it makes sense to keep the app code simple and move complexity to test code. Inlined the function and moved to test cases to the new file.
| // Ideally we just use ACTIONS_CACHE_URL | ||
| const baseUrl: string = ( | ||
| process.env['ACTIONS_CACHE_URL'] || | ||
| process.env['ACTIONS_RUNTIME_URL'] || |
There was a problem hiding this comment.
Breaking change here. If we're going to remove this, we need to bump the package version to 2.0.0
There was a problem hiding this comment.
This should not be a breaking change as we are already setting ACTIONS_CACHE_URL in runner based on the presence of Artifact Cache service https://github.com/github/actions-dotnet/blob/9925858070aba79b8c5f8be793f58e1117ecd03d/Actions/Runtime/Sdk/Server/TaskHub.cs#L3484 and https://github.com/actions/runner/blob/main/src/Runner.Worker/Handlers/NodeScriptActionHandler.cs#L53-L56
There was a problem hiding this comment.
Regardless of what we do in the runner, it technically is still a breaking change and we should update the major version accordingly. It's not a big deal. Alternatively, we can just keep the check for ACTIONS_RUNTIME_URL.
There was a problem hiding this comment.
While technically yes it is a breaking change, pretty sure it is not so in practical terms. In hosted Actions, ACTIONS_CACHE_URL is set and no-one would be using the fallback. In GHES, caching was not available and hence need not worry about older server/runner versions.
Hence we should be okay to do without extra work of a major version change.
There was a problem hiding this comment.
updated for major release to be safer side
Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com>
This reverts commit af84eba.
| process.env['ACTIONS_RUNTIME_URL'] || | ||
| '' | ||
| ).replace('pipelines', 'artifactcache') | ||
| const baseUrl: string = process.env['ACTIONS_CACHE_URL'] || '' |
There was a problem hiding this comment.
Actually we don't need it but this safeguard was there from beginning so i didn't remove it
This PR introduced a function that can be used to check the presence of Artifact Cache service using the presence of ACTION_CACHE_URL env var. This var we only set in runner if Artifact Cache is present.
These are changes required on the package consumer side: actions/cache#774
We have tested it for GHES, where AC is only available in version >= 3.5
In 3.4 where AC is not present
In GHES, where AC is present
Linked: https://github.com/github/c2c-actions/issues/4065