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

feat!: rewrite in TypeScript with CacheMap support #25

Merged
merged 11 commits into from
Mar 31, 2024

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Mar 29, 2024

This pull request rewrites the action in TypeScript and adds support for cache-map that gets a string of files that need to be injected as a JSON string. This makes it possible to inject multiple directories in one call and simplifies the usage.

It also makes it possible to run the script outside GitHub Actions (e.g. for S3) or locally using command line arguments.

BREAKING cache-source and cache-target are deprecated in favour of cache-map that expects a JSON string.

I have bumped the version to v3.0.0

Fixes #10
Closes #24
Closes #22
Allows implementation of #16

      - name: Cache
        uses: actions/cache@v3
        id: cache
        with:
          path: |
            var-cache-apt
            var-lib-apt
          key: cache-${{ hashFiles('.github/workflows/test/Dockerfile') }}

      - name: inject cache into docker
        uses: ./
        with:
          cache-map: |
            {
              "var-cache-apt": "/var/cache/apt",
              "var-lib-apt": "/var/lib/apt"
            }
          skip-extraction: ${{ steps.cache.outputs.cache-hit }}

@aminya aminya force-pushed the rewrite branch 3 times, most recently from d8b37a6 to b8a90d3 Compare March 29, 2024 11:40
src/index.ts Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@aminya aminya force-pushed the rewrite branch 2 times, most recently from 4a43de2 to b8ea280 Compare March 30, 2024 01:59
@aminya
Copy link
Contributor Author

aminya commented Mar 30, 2024

Recovered the cache-source and cache-target options but with deprecation messages. Also, added an example of running this outside GitHub Actions.

dist/index.js Outdated Show resolved Hide resolved
@AkihiroSuda
Copy link
Member

https://github.com/reproducible-containers/buildkit-cache-dance/actions/runs/8490107723/job/23260959438?pr=25

Post job cleanup.

FROM busybox:1
COPY buildstamp buildstamp
RUN --mount=type=cache,target=/var/cache/apt     mkdir -p /var/dance-cache/     && cp -p -R /var/cache/apt/. /var/dance-cache/ || true


node:internal/process/esm_loader:40
      internalBinding('errors').triggerUncaughtException(
                                ^
tar: Skipping to next header
tar: Exiting with failure status due to previous errors

(Use `node --trace-uncaught ...` to show where the exception was thrown)

Node.js v[2](https://github.com/reproducible-containers/buildkit-cache-dance/actions/runs/8490107723/job/23260959438?pr=25#step:11:2)0.8.1

@aminya
Copy link
Contributor Author

aminya commented Mar 30, 2024

The error you sent doesn't happen on my fork. But I pushed a fix regardless for more robust error handling
https://github.com/aminya/buildkit-cache-dance/actions

@AkihiroSuda
Copy link
Member

The test passes when some cache is present on GHA, but it still fails after pruning the cache via https://github.com/reproducible-containers/buildkit-cache-dance/actions/caches

@AkihiroSuda
Copy link
Member

Also, please remove the "Merge" commit (use git rebase, not git merge), and consider squashing the commits

aminya added 7 commits March 30, 2024 04:06
BREAKING `cache-source` and `cache-target` are removed in favour of `cache-map` that expects a JSON string

Signed-off-by: Amin Yahyaabadi <[email protected]>
Signed-off-by: Amin Yahyaabadi <[email protected]>
Signed-off-by: Amin Yahyaabadi <[email protected]>
@aminya
Copy link
Contributor Author

aminya commented Mar 30, 2024

The test passes when some cache is present on GHA, but it still fails after pruning the cache via reproducible-containers/buildkit-cache-dance/actions/caches

Okay, found the bug. The stdout was not being captured properly.

@aminya
Copy link
Contributor Author

aminya commented Mar 31, 2024

Simplified and fixed the docker to tar data piping issue. It now works on my fork with cache pruned as well:
https://github.com/aminya/buildkit-cache-dance/actions/runs/8494967106/job/23270766398

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 0fc239d into reproducible-containers:main Mar 31, 2024
3 checks passed
@strophy
Copy link

strophy commented Mar 31, 2024

Thanks @aminya this is a really significant improvement!

@aminya
Copy link
Contributor Author

aminya commented Mar 31, 2024

You're welcome!

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.

Process multiple targets in single action call and support S3 backend
3 participants