-
Notifications
You must be signed in to change notification settings - Fork 583
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
Cache on ghes #363
Changes from 11 commits
c1f6351
0c2c19b
e6ed85b
3dff462
d46988a
f477cc3
f3f9e75
74a60ed
c5c30f5
298ed7c
85413f7
fc72fbd
3bc0f07
1a05397
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1,3 +1,5 @@ | ||||||||||
import * as cache from '@actions/cache'; | ||||||||||
import * as core from '@actions/core'; | ||||||||||
import fs from 'fs'; | ||||||||||
import * as path from 'path'; | ||||||||||
import * as semver from 'semver'; | ||||||||||
|
@@ -99,3 +101,21 @@ 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 a version >= 3.5, please check with your GHES admin if the Actions cache service is enabled or not.' | ||||||||||
); | ||||||||||
Comment on lines
+108
to
+110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it necessary to throw an error here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per the previous code, we were throwing an error in the GHES scenario https://github.com/actions/setup-python/blob/main/src/setup-python.ts#L15. Now we don't want to change end-user behaviour and this condition is similar to that that where workflow is running on GHES and workflow has explicitly asked for the cache using |
||||||||||
} else { | ||||||||||
core.warning( | ||||||||||
'An internal error has occurred in cache backend. Please check https://www.githubstatus.com/ for any ongoing issue in actions.' | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this message accurate? It looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If my understanding is correct, I would say something like
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. Let me update the PR with changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||||||
); | ||||||||||
} | ||||||||||
|
||||||||||
return false; | ||||||||||
} | ||||||||||
|
||||||||||
return true; | ||||||||||
} |
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.
Could you please change it to the planning version of the setup-python action?
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.
@dmitry-shibanov, Sorry I am not aware of the planned version. Could please share what is planned version.
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.
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
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.
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?