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

fix: use actions/runner hashfiles in container #1940

Merged
merged 5 commits into from
Oct 3, 2023

Conversation

ChristopherHX
Copy link
Contributor

@ChristopherHX ChristopherHX commented Aug 1, 2023

Previously hashfiles ran on the host,
this don't work for container generated content

Primary for github-act-runner and act_runner downstream projects, because they don't have a checkout on the host.

The src of hashfiles can be found in actions/runner. I don't see any legal issues both act and actions/runner use the same type of license.

Fixes #2014

Previously hashfiles ran on the host,
this don't work for container generated content
@ChristopherHX ChristopherHX requested a review from a team as a code owner August 1, 2023 19:26
@mergify
Copy link
Contributor

mergify bot commented Aug 1, 2023

@ChristopherHX this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Aug 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 3 0 0.09s
✅ REPOSITORY gitleaks yes no 3.35s
✅ REPOSITORY git_diff yes no 0.22s
✅ REPOSITORY grype yes no 9.94s
✅ REPOSITORY secretlint yes no 3.85s
✅ REPOSITORY trivy-sbom yes no 0.87s
✅ REPOSITORY trufflehog yes no 25.62s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@mergify
Copy link
Contributor

mergify bot commented Aug 1, 2023

@ChristopherHX this pull request has failed checks 🛠

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #1940 (b23d208) into master (4989f44) will increase coverage by 0.49%.
Report is 248 commits behind head on master.
The diff coverage is 60.58%.

@@            Coverage Diff             @@
##           master    #1940      +/-   ##
==========================================
+ Coverage   61.22%   61.71%   +0.49%     
==========================================
  Files          46       52       +6     
  Lines        7141     8544    +1403     
==========================================
+ Hits         4372     5273     +901     
- Misses       2462     2848     +386     
- Partials      307      423     +116     
Files Coverage Δ
pkg/common/executor.go 51.69% <100.00%> (+1.69%) ⬆️
pkg/container/docker_cli.go 82.23% <ø> (ø)
pkg/container/docker_logger.go 52.08% <ø> (ø)
pkg/runner/job_executor.go 88.48% <100.00%> (+0.79%) ⬆️
pkg/runner/step_action_local.go 93.54% <100.00%> (ø)
pkg/runner/step_action_remote.go 91.56% <100.00%> (+0.65%) ⬆️
pkg/runner/step_docker.go 93.18% <100.00%> (ø)
pkg/container/file_collector.go 37.30% <0.00%> (ø)
pkg/container/util.go 0.00% <0.00%> (ø)
pkg/container/docker_build.go 60.00% <80.00%> (+1.02%) ⬆️
... and 30 more

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mergify mergify bot removed the needs-work Extra attention is needed label Aug 1, 2023
@mergify mergify bot requested a review from a team August 4, 2023 02:24
Co-authored-by: Jason Song <i@wolfogre.com>
@cplee
Copy link
Contributor

cplee commented Aug 8, 2023

I don't like vendoring hashfiles like this - make maintenance challenging. Is there other options? For example, can we use docker to copy the file out of container and then hash using existing function?

@ChristopherHX
Copy link
Contributor Author

ChristopherHX commented Aug 8, 2023

Yes we could copy act itself into the container and exec act, if we add a hashfiles entrypoint.

EDIT This creates a problem for my github-act-runner project, because it also need to implement that entrypoint and act may be 10MB or larger.

For example, can we use docker to copy the file out of container and then hash using existing function?

This would be inefficient, imagine you have a 5GB repo and need to copy it out of the container everytime you call hashfiles.

@ChristopherHX
Copy link
Contributor Author

ChristopherHX commented Aug 8, 2023

You know actions/runner doesn't have this problem, because they use bind mounts. The workspace and home folder is accessible by the host (but not in act)

While actions/runner has problems with ssh and tcp protocols (without hacks) this just works in act due to using docker volumes and file exchange by using docker cp.

@ChristopherHX
Copy link
Contributor Author

We could create a minimal golang build of the existing hashfiles implementation as a cli tool.

  • I think it would be smaller than act, by not needing most code of act and it's dependencies
  • We can just download it into a docker volume (Docker Container) / to the cache folder (Host Environment).

@wolfogre
Copy link
Member

wolfogre commented Aug 9, 2023

We could create a minimal golang build of the existing hashfiles implementation as a cli tool.

How can we ensure that the a cli tool can execute in all host environments? There may be some arch or os that are very unpopular, although they do not fully support running act, users may run some simple steps on them.

Since actions always assume that the environment has nodejs, I prefer this PR which uses js to compute hash.

@ChristopherHX
Copy link
Contributor Author

ChristopherHX commented Aug 9, 2023

How can we ensure that the a cli tool can execute in all host environments?

Yes this is a problem, we cannot enshure this. I have plan9, solaris, openbsd and freebsd builds.
This also mean we cannot easily provide nodejs for docker container as external tool like actions/runner (node depends also on distro libc, alpine vs. glibc / golang depends only on os and arch).

In my runner.server code I inspect the docker image used to run to get it's docker platform and bind mount hopefully matching external tools and download them on use if they are missing. In .net I also runtime detect the os, arch and alpine via .net apis.
But yes, a lot of platforms like powerpc64, freebsd and so on are missing.
Also if I create a new port of act we need also a cli tool version for that platform.

I agree with wolfogre this sounds to be more maintenance work after all to use this as an alternative.

I just vendored this file https://github.com/actions/runner/blob/main/src/Misc/layoutbin/hashFiles/index.js (removed trailing spaces and added a newline at the end to satify the linter)

Sourcecode is here: https://github.com/actions/runner/tree/main/src/Misc/expressionFunc/hashFiles

@mergify mergify bot merged commit 7c7d80e into master Oct 3, 2023
10 checks passed
@mergify mergify bot deleted the run-hash-files-in-container branch October 3, 2023 22:56
jmikedupont2 pushed a commit to meta-introspector/act that referenced this pull request Mar 10, 2024
* fix: use actions/runner hashfiles in container

Previously hashfiles ran on the host,
this don't work for container generated content

* fix: lint

* fix: lint

* fix assign follow symlink flag

Co-authored-by: Jason Song <i@wolfogre.com>

---------

Co-authored-by: Jason Song <i@wolfogre.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hashFiles does not resolve paths relative to GITHUB_WORKSPACE
3 participants