FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Caching on GHES by tiwarishub · Pull Request #308 · actions/setup-java · GitHub

/ setup-java Public

Caching on GHES - #308

Merged
brcrista merged 7 commits into
actions:mainfrom
tiwarishub:cacheOnGhes
Mar 31, 2022
ConversationCommits (7)ChecksFiles changed
Merged

Caching on GHES#308
brcrista merged 7 commits into
actions:mainfrom
tiwarishub:cacheOnGhes

Conversation

tiwarishub commented Mar 29, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

With new changes. Without AC on GHES

With AC on GHES
save

restore

without choosing the cache option on GHES with AC

On dotcom

save

cache hit

without cache option

Related issue:
#307

Check list:

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

tiwarishub changed the title Cache on ghes Caching on GHES Mar 30, 2022
tiwarishub marked this pull request as ready for review March 30, 2022 06:26
tiwarishub requested review from a team and bishal-pdMSFT March 30, 2022 06:27
dmitry-shibanov approved these changes Mar 30, 2022
View reviewed changes
Comment thread package.json Outdated
{
"name": "setup-java",
"version": "2.0.0",
"version": "2.0.1",

dmitry-shibanov Mar 30, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think we need to update the version according to the next release version.

tiwarishub Mar 30, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes, we are planning to release after this PR merge

dmitry-shibanov Mar 30, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could you please change version 2.0.1 to the planing one ?

tiwarishub Mar 30, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@dmitry-shibanov , please share planned version

brcrista reviewed Mar 30, 2022
View reviewed changes
Comment thread 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.'

brcrista Mar 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

brcrista Mar 30, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tiwarishub Mar 30, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done

brcrista reviewed Mar 30, 2022
View reviewed changes
Comment thread src/util.ts Outdated
);
} else {
core.warning(
'This runner is not configured to use the cache service. Caching will be skipped'

brcrista Mar 30, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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'

tiwarishub Mar 31, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Updated with correct message

brcrista commented Mar 31, 2022

Copy link
Copy Markdown
Contributor

Looks like a test is failing: https://github.com/actions/setup-java/runs/5769289844?check_suite_focus=true#step:7:461

tiwarishub commented Mar 31, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Looks like a test is failing: https://github.com/actions/setup-java/runs/5769289844?check_suite_focus=true#step:7:461

Yes this is failing on master as well #309 https://github.com/actions/setup-java/runs/5773757120?check_suite_focus=true#step:7:404

Update:
NowCI fixed on master #310

brcrista merged commit dc1a9f2 into actions:main Mar 31, 2022
dmitry-shibanov mentioned this pull request Apr 7, 2022
Closed
2 tasks
tdfacer pushed a commit to ifit/setup-java that referenced this pull request Oct 7, 2025
Caching on GHES (actions#308)
139020e 
* initial changes

* review comments

* updated with correct message

* linting

* update version

* updated version
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bishal-pdMSFT Awaiting requested review from bishal-pdMSFT
aparna-ravindra Awaiting requested review from aparna-ravindra
3 more reviewers
dmitry-shibanov dmitry-shibanov approved these changes
brcrista brcrista approved these changes
marko-zivic-93 marko-zivic-93 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL