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

Caching on GHES #452

Merged

Conversation

dmitry-shibanov
Copy link
Contributor

@dmitry-shibanov dmitry-shibanov commented Mar 30, 2022

Description:
In scope of this pull request we add support for caching on GHES.
Related issue:
#450

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@dmitry-shibanov dmitry-shibanov requested a review from a team as a code owner Mar 30, 2022
@dmitry-shibanov
Copy link
Contributor Author

dmitry-shibanov commented Mar 30, 2022

Hello everyone. Could you please take a look at this pull request.
cc: @tiwarishub @brcrista

Copy link

@tiwarishub tiwarishub left a comment

Thank you for taking this PR 🙇 . I have left few comments

@@ -102,6 +103,9 @@ describe('cache-restore', () => {

// cache-utils
getCommandOutputSpy = jest.spyOn(utils, 'getCommandOutput');

Choose a reason for hiding this comment

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

Is this change and L53 change is required? Because I think in restore restoreCache function we don't check isCacheActionAvailable functions. This we are checking in main.ts

Copy link
Contributor Author

@dmitry-shibanov dmitry-shibanov Mar 31, 2022

Choose a reason for hiding this comment

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

Done. Thank you.

@@ -70,6 +71,9 @@ describe('run', () => {

// utils
getCommandOutputSpy = jest.spyOn(utils, 'getCommandOutput');

Choose a reason for hiding this comment

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

Same as above comment. Is this change required here

Copy link
Contributor Author

@dmitry-shibanov dmitry-shibanov Mar 31, 2022

Choose a reason for hiding this comment

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

Done. Thank you.

@@ -51,7 +40,35 @@ describe('cache-utils', () => {
});
});

it('isCacheFeatureAvailable is false', () => {

Choose a reason for hiding this comment

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

in the description, Please add whether the scenario is of GHES or not.

Copy link
Contributor Author

@dmitry-shibanov dmitry-shibanov Mar 31, 2022

Choose a reason for hiding this comment

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

Done. Thank you.

);
});

it('isCacheFeatureAvailable is false', () => {

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

@dmitry-shibanov dmitry-shibanov Mar 31, 2022

Choose a reason for hiding this comment

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

Done. Thank you.

export function isCacheFeatureAvailable(): boolean {
if (!cache.isFeatureAvailable()) {
if (isGhes()) {
logWarning(

Choose a reason for hiding this comment

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

I would prefer here to throw an error instead of a warning. Because this keeps it consistent with other setup-* actions https://github.com/actions/setup-java/pull/308/files#diff-3294a832ea2276e554177e0b3007cc2d401c082912c7fbde49fa09141bf1aed1R87-R103

Copy link
Contributor Author

@dmitry-shibanov dmitry-shibanov Mar 31, 2022

Choose a reason for hiding this comment

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

Done. Thank you.

src/main.ts Outdated
@@ -46,7 +46,7 @@ export async function run() {
}

if (cache) {
if (isGhes()) {
if (!isCacheFeatureAvailable()) {
throw new Error('Caching is not supported on GHES');

Choose a reason for hiding this comment

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

Let's not throw the error here (if we can throw it from isCacheFeatureAvailable function ). We can return if the Cache feature is not available

Copy link
Contributor Author

@dmitry-shibanov dmitry-shibanov Mar 31, 2022

Choose a reason for hiding this comment

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

Done. Thank you.

);
} else {
core.warning(
'An internal error has occurred in cache backend. Please check https://www.githubstatus.com/ for any ongoing issue in actions.'
Copy link
Contributor

@brcrista brcrista Mar 30, 2022

Choose a reason for hiding this comment

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

@dmitry-shibanov in actions/setup-python#363 we ended up changing this warning:

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

@dmitry-shibanov dmitry-shibanov Mar 31, 2022

Choose a reason for hiding this comment

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

Done. Thank you.

@brcrista brcrista merged commit bacd6b4 into actions:main Mar 31, 2022
98 checks passed
@dmitry-shibanov dmitry-shibanov mentioned this pull request Apr 5, 2022
1 task
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.

None yet

5 participants