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
Merged

Cache on ghes #363

merged 14 commits into from Mar 31, 2022

Conversation

tiwarishub
Copy link
Contributor

@tiwarishub tiwarishub commented Mar 29, 2022

Description:
This PR adds caching support in setup-python for GHES with version 3.5. It checks the presence of Actions cache service(which is used for caching dependencies in GHES and dotcom) using recent @actions/cache tookit package function i.e isFeatureAvailable. This same function can be applied to dotcom scenario (github.com) so this function can be safely applied to dotcom scenario as well.
I have tested the below scenarios

  1. When AC (Action cache) service is not present in GHES

image

  1. AC present in GHES. Cache save

image

  1. AC present in GHES. restored saved cache.

image

  1. Without choosing the cache option in GHES

image

  1. On Dotcom, with the cache option chosen
    save https://github.com/tiwarishub/python-caching/actions/runs/2060198424
    image

restore https://github.com/tiwarishub/python-caching/runs/5741964174?check_suite_focus=true
image

  1. On Dotcom, without cache option is chosen https://github.com/tiwarishub/python-caching/runs/5741986713?check_suite_focus=true
    image

Note
v3 version of setup-python is not published to GHES 3.4 so with this we will also release v3 version of setup-python. Please let us know if there is any issue with that.

Related issue:
#362

Check list:

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

@tiwarishub
Copy link
Contributor Author

@tiwarishub tiwarishub commented Mar 29, 2022

Few CI checks are failing but I believe this is failing on master so I think it ok to ignore it?
I have not added the workflow runs links of the GHES scenarios. If anyone want please ping me on slack then it will GHES host and password

@tiwarishub tiwarishub marked this pull request as ready for review Mar 29, 2022
@tiwarishub tiwarishub requested a review from as a code owner Mar 29, 2022
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated
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

@brcrista brcrista Mar 29, 2022

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

@brcrista brcrista Mar 29, 2022

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

@tiwarishub tiwarishub Mar 29, 2022

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

@tiwarishub tiwarishub Mar 29, 2022

Choose a reason for hiding this comment

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

Updated the PR

Copy link
Contributor

@brcrista brcrista left a comment

I think we should consider using warnings instead of errors for caching failures.

Copy link

@bishal-pdMSFT bishal-pdMSFT left a comment

LGTM

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.'
);
Copy link
Contributor

@vsafonkin vsafonkin Mar 30, 2022

Choose a reason for hiding this comment

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

Is it necessary to throw an error here?

Copy link
Contributor Author

@tiwarishub tiwarishub Mar 30, 2022

Choose a reason for hiding this comment

The 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 cache option but it has not enabled the cache. we had similar discussion here #363 (comment)

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "setup-python",
"version": "2.2.2",
"version": "2.2.3",
Copy link
Contributor

@dmitry-shibanov dmitry-shibanov Mar 30, 2022

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

@tiwarishub tiwarishub 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, Sorry I am not aware of the planned version. Could please share what is planned version.

Copy link
Contributor

@dmitry-shibanov dmitry-shibanov Mar 30, 2022

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

@tiwarishub tiwarishub Mar 30, 2022

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?

src/utils.ts Outdated
);
} 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.

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

@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.

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

@tiwarishub tiwarishub Mar 30, 2022

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

@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.

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

@tiwarishub tiwarishub Mar 30, 2022

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

@tiwarishub tiwarishub Mar 30, 2022

Choose a reason for hiding this comment

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

done

@brcrista brcrista mentioned this pull request Mar 30, 2022
2 tasks
@brcrista brcrista mentioned this pull request Mar 30, 2022
2 tasks
Copy link
Contributor

@marko-zivic-93 marko-zivic-93 left a comment

All issues we noticed were referenced :) Everything seems ok now :)

@brcrista brcrista merged commit 05fb98d into actions:main Mar 31, 2022
84 of 102 checks passed
@tiwarishub tiwarishub mentioned this pull request Apr 1, 2022
2 tasks
adilosa added a commit to adilosa/setup-python that referenced this issue Apr 19, 2022
* upstream/main: (33 commits)
  Fix virtual-env toolcache links
  Updated @actions/cache (actions#382)
  ci(workflow): add 'npm' cache for actions/setup-node in .github/workflows (actions#379)
  Cache hit output (actions#373)
  Add pyton-version to setup PyPy output (actions#365)
  Rework pipenv caching test (actions#375)
  Update README.md to fix setup-python version  in example (actions#368)
  dist fix (actions#367)
  Cache on ghes (actions#363)
  Update dist
  Use `\n` instead of `os.EOL`
  Update dist
  Initialise pyproject.toml
  Build and format
  Remove console.log
  Remove unused file
  Reduce test matrix
  Parse values from poetry
  Release
  Add more tests
  ...
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

6 participants