Skip to content

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

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