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

Implementation of caching functionality for setup-go action #228

Merged
merged 63 commits into from May 25, 2022

Conversation

IvanZosimov
Copy link
Contributor

@IvanZosimov IvanZosimov commented Apr 26, 2022

In the scope of this pull request, the possibility of caching go dependency files and compiler's build outputs was added. Such input parameters as cache and cache-dependency-path were added.

For now, cache input will accept the following values:

true - enable caching for go dependency files and build outputs
false - disable caching for go dependency files and build outputs (default value)

cache-dependency-path input is used to specify the path to a dependency file - go.sum

Description
Action will try to search go.sum file in the repository root or in any other location which is specified in cache-dependency-path input and throw an error if no one is found. The hash of the found file will be used as a cache key ( the same approach as actions/cache recommends). The following key cache will be used: ${{ runner.os }}-go${{ go-version }}-${{ hashFiles('<go.sum-path>') }}

Examples of use-cases:

  • cache input only
steps:
- uses: actions/checkout@v3
- uses: actions/setup-go@v3
  with:
    go-version: '18'
    cache: true
  • cache along with cache-dependency-path
steps:
- uses: actions/checkout@v3
- uses: actions/setup-go@v3
 with:
   go-version: '18'
   cache: true
   cache-dependency-path: **/go.sum

Linked ADR: #217

Note:
It's not breaking change because it requires an additional field and nothing changes by default.

What's done:

  • Implementation of go dependency files and compiler's build outputs caching
  • Adding unit tests for caching implementation
  • Updating documentation
  • Updating licenses

Ivan Zosimov (Akvelon INC) and others added 30 commits Feb 21, 2022
Such inputs as 'cache' and 'cache-dependency-path' were added along with
their descriptions.
Divided building procedure into two, one for main job, another for
post-job.
Such files as cache-restore.ts, cache-utils.ts, constants.ts,
cache-save.ts we added. Main.ts file now incorporates logic for using
files mentioned above.
If caching is not enabled by action.yml input, save of the cache won't
occur in the post-job.
Changes were applied to some debug messages.
Change modules-manager to package-manager in action.yml file.
File package-managers.ts was added.
@IvanZosimov IvanZosimov requested a review from a team as a code owner Apr 26, 2022
);

if (nonExistingPaths.length === cachePaths.length) {
throw new Error(`No cache folders exist on disk`);
Copy link
Contributor

@dmitry-shibanov dmitry-shibanov Apr 26, 2022

Choose a reason for hiding this comment

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

No cache folders exist on disk -> 'There are no cache folders on the disk'

Copy link
Contributor Author

@IvanZosimov IvanZosimov Apr 27, 2022

Choose a reason for hiding this comment

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

Solved, review again, please.

packageManagerInfo: PackageManagerInfo
) => {
let pathList = await Promise.all(
packageManagerInfo.cacheFolderCommandList.map(async command =>
Copy link
Contributor

@dmitry-shibanov dmitry-shibanov Apr 26, 2022

Choose a reason for hiding this comment

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

Do we need async here ?

Copy link
Contributor Author

@IvanZosimov IvanZosimov Apr 27, 2022

Choose a reason for hiding this comment

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

Solved, review again, please.

README.md file was updated, removed async declaration in cache-utils
file and changed the error message in cache-save file.
);
}

const primaryKey = `${platform}-go${versionSpec}-${fileHash}`;
Copy link
Contributor

@brcrista brcrista Apr 29, 2022

Choose a reason for hiding this comment

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

I think it would be great to use a standard format for cache keys.

  • For Node, it looks like: node-cache-${platform}-${packageManager}-${fileHash}
  • For Python / Pip, it looks like ${this.CACHE_KEY_PREFIX}-${process.env['RUNNER_OS']}-python-${this.pythonVersion}-${this.packageManager}-${hash}

Now I know those aren't the same, but I think we should follow one or the other so we don't drift so much. That will help if we're ever able to consolidate the setup- caching logic in some package.

@@ -0,0 +1,62 @@
import * as cache from '@actions/cache';
Copy link
Contributor

@brcrista brcrista Apr 29, 2022

Choose a reason for hiding this comment

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

It seems like the caching implementation has a lot in common with other setup- actions. Many of the functions are similar to those used in setup-node, for example.

I think it would be much better either to put these in @actions/cache. Or we could create a shared "@actions/setup" package that encapsulates all the common logic among setup- actions. That will make sure all setup- actions behave consistently.

@actions actions deleted a comment from momeyman May 19, 2022
@actions actions deleted a comment from momeyman May 19, 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

6 participants