Skip to content

WIP: Enable Non-Root Runner Scenarios#666

Closed
JustinGrote wants to merge 6 commits intonektos:masterfrom
JustinGrote:feature/EnableNonRootWorkers
Closed

WIP: Enable Non-Root Runner Scenarios#666
JustinGrote wants to merge 6 commits intonektos:masterfrom
JustinGrote:feature/EnableNonRootWorkers

Conversation

@JustinGrote
Copy link
Copy Markdown
Contributor

@JustinGrote JustinGrote commented May 5, 2021

Currently act requires the docker container to have root privileges to work because it both binds and mounts to the "same path" as the host.

While the bind method is useful if using act as a task runner, the mount method will fail in non-root containers.

In order to more accurately test Github Actions for both permission and pathing errors, this PR makes mounts use the runner workdir by default and sets permissions accordingly. Binds with the -b flag will be unaffected and will continue to use the same path as what is on the container.

It will also add a parameter to customize the starting container workdir separately from the host workdir, if there is such a situation where you dont' want to use the default (usually /home/runner/workdir)

Example that this will fix

Try a simple action that does mkdir -p /my/random/path. This will work just fine in act, but fail in Github Actions, and you won't find out till you run it in github actions.

@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented May 5, 2021

@JustinGrote this pull request has failed checks 🛠

@mergify mergify Bot added the needs-work Extra attention is needed label May 5, 2021
@catthehacker
Copy link
Copy Markdown
Member

There is quite a mix of old code and different branches from your fork

@JustinGrote
Copy link
Copy Markdown
Contributor Author

@catthehacker yeah as mentioned it's draft and WIP, I just rebased to my ActForWindows branch and haven't cleaned up the stuff that the merge missed.

@JustinGrote JustinGrote force-pushed the feature/EnableNonRootWorkers branch from 8c1c8e6 to 412e0c7 Compare May 5, 2021 16:24
@pull-request-size pull-request-size Bot added size/S and removed size/L labels May 5, 2021
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented May 5, 2021

@JustinGrote this pull request has failed checks 🛠

2 similar comments
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented May 5, 2021

@JustinGrote this pull request has failed checks 🛠

@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented May 6, 2021

@JustinGrote this pull request has failed checks 🛠

@JustinGrote JustinGrote force-pushed the feature/EnableNonRootWorkers branch from f517040 to fd8f77c Compare May 6, 2021 16:13
@pull-request-size pull-request-size Bot added size/M and removed size/S labels May 6, 2021
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented May 6, 2021

@JustinGrote this pull request has failed checks 🛠

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2021

Codecov Report

Merging #666 (42b0385) into master (0f04942) will increase coverage by 0.84%.
The diff coverage is 58.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
+ Coverage   49.27%   50.11%   +0.84%     
==========================================
  Files          23       23              
  Lines        2401     2542     +141     
==========================================
+ Hits         1183     1274      +91     
- Misses       1090     1132      +42     
- Partials      128      136       +8     
Impacted Files Coverage Δ
pkg/container/docker_run.go 1.74% <0.00%> (-0.19%) ⬇️
pkg/common/git.go 54.67% <33.92%> (-5.12%) ⬇️
pkg/model/planner.go 34.56% <41.37%> (+1.48%) ⬆️
pkg/model/workflow.go 30.71% <60.00%> (+5.00%) ⬆️
pkg/container/docker_pull.go 36.17% <64.70%> (+17.98%) ⬆️
pkg/runner/step_context.go 73.00% <74.79%> (+4.05%) ⬆️
pkg/runner/run_context.go 79.74% <94.82%> (+3.33%) ⬆️
pkg/runner/runner.go 76.92% <100.00%> (+0.45%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef0da2a...42b0385. Read the comment docs.

@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented May 6, 2021

@JustinGrote this pull request has failed checks 🛠

@JustinGrote JustinGrote force-pushed the feature/EnableNonRootWorkers branch from bb8d0eb to 9db1599 Compare May 6, 2021 17:13
@mergify mergify Bot removed the needs-work Extra attention is needed label May 6, 2021
@JustinGrote JustinGrote marked this pull request as ready for review May 6, 2021 17:33
@JustinGrote JustinGrote requested a review from a team as a code owner May 6, 2021 17:33
@JustinGrote JustinGrote marked this pull request as draft May 6, 2021 17:33
@JustinGrote JustinGrote marked this pull request as ready for review May 6, 2021 17:49
@JustinGrote
Copy link
Copy Markdown
Contributor Author

JustinGrote commented May 6, 2021

Requesting design review and that you agree with the approach before I finish tests.
@catthehacker @cplee ping for review, thanks :)

@JustinGrote JustinGrote force-pushed the feature/EnableNonRootWorkers branch from a0bbe3d to 42b0385 Compare May 10, 2021 22:53
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented May 24, 2021

@JustinGrote this pull request is now in conflict 😩

@mergify mergify Bot added the conflict PR has conflicts label May 24, 2021
@github-actions
Copy link
Copy Markdown
Contributor

PR is stale and will be closed in 14 days unless there is new activity

@github-actions github-actions Bot added the stale label Jun 24, 2021
@github-actions github-actions Bot closed this Jul 9, 2021
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.

2 participants