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 #308

Merged
merged 7 commits into from Mar 31, 2022
Merged

Caching on GHES #308

merged 7 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-java for GHES with version 3.5. It checks the presence of the Actions cache service(which is used for caching dependencies in GHES and dotcom) using the recent @actions/cache toolkit 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
on GHES
Current behaviour
image

With new changes. Without AC on GHES
image

With AC on GHES
save
image
restore
image

without choosing the cache option on GHES with AC
image

On dotcom

save
image

cache hit
image

without cache option
image

Related issue:
#307

Check list:

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

@tiwarishub tiwarishub changed the title Cache on ghes Caching on GHES Mar 30, 2022
@tiwarishub tiwarishub marked this pull request as ready for review March 30, 2022 06:26
@tiwarishub tiwarishub requested review from a team and bishal-pdMSFT March 30, 2022 06:27
package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "setup-java",
"version": "2.0.0",
"version": "2.0.1",
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 we need to update the version according to the next release version.

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, we are planning to release after this PR merge

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 version 2.0.1 to the planing one ?

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 , please share planned version

src/util.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

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.

done

src/util.ts Outdated
);
} else {
core.warning(
'This runner is not configured to use the cache service. Caching will be skipped'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update to the same thing from actions/setup-python#363?

Suggested change
'This runner is not configured to use the cache service. Caching will be skipped'
'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.

Updated with correct message

@brcrista
Copy link
Contributor

@tiwarishub
Copy link
Contributor Author

tiwarishub commented Mar 31, 2022

@brcrista brcrista merged commit dc1a9f2 into actions:main Mar 31, 2022
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