Separate Container Workdir from host Workdir

[JustinGrote]

This PR started as a WSL2 fix but the pathing issue was so pervasive it expanded in scope a bit to meet the requirement.

A summary of changes:

• Adds path translation for WSL2 paths in order to enable act to function on Windows again.
• Refactor Binds and Mounts to a new function to coordinate either/or based on the BindWorkDir setting
• Add ContainerWorkDir() function to fetch the relative container function. This currently has the same behavior as before on Linux but translates to the WSL2 path on windows. In a future PR I will expand this to allow a command switch so that the container can as closely mimic the Github Actions runner paths as possible.

Tests:

• Changed Integration tests to use BindWorkDir: false instead of true to more closely match the most common act behavior (since bindworkdir is false by default). Tested to work both ways, should probably expand them as such
• Removed duplicate integration tests, not sure if this was done on purpose to make sure they are idempotent, this should be more clearly done with a loop instead
• Added the ability for tests to read from the .secrets file so that GITHUB_TOKEN can be specified for actions like action/checkout and be tested locally and not just inside github actions
• Removed checkout from workdir test, it's not necessary for the purpose of the test

Resolves #587

[codecov]

Codecov Report

Merging #635 (d83e7b0) into master (020d6a6) will decrease coverage by 0.35%.
The diff coverage is 76.05%.

@@            Coverage Diff             @@
##           master     #635      +/-   ##
==========================================
- Coverage   49.62%   49.27%   -0.36%     
==========================================
  Files          23       23              
  Lines        2380     2401      +21     
==========================================
+ Hits         1181     1183       +2     
- Misses       1072     1090      +18     
- Partials      127      128       +1     

Impacted Files	Coverage Δ
pkg/runner/runner.go	76.47% <37.50%> (-17.82%)	⬇️
pkg/runner/step_context.go	68.94% <80.76%> (-1.92%)	⬇️
pkg/runner/run_context.go	76.40% <93.10%> (-0.17%)	⬇️

---

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 020d6a6...d83e7b0. Read the comment docs.

[JustinGrote]

@catthehacker ready to review.

FYI you need to update your codecov action to include a base target, otherwise this is pretty much always going to reject through no fault of my own because the codebase outside of my PR is being tested as well.

[catthehacker]

Don't mind codecov, it does it job fine even when failing 😋
Will take a look later

[JustinGrote]

Don't mind codecov, it does it job fine even when failing 😋
Will take a look later

Thanks, since you had codecov as a check I thought it might block the merge but since you're an admin I guess you can just force merge it anyways :)

[catthehacker]

Please no

[JustinGrote]

See answer above for run tests

[JustinGrote]

Reverted to draft to complete tests.

[JustinGrote]

@catthehacker unit tests added for new functions, this should be ready to merge. Tests pass on my machine but the docker setup barfs in Windows Github Actions runner, I'll try to make a separate PR to enable Windows PR tests to run.

[catthehacker]

@JustinGrote it's not possible to run Docker on Windows GitHub runner currently.

[JustinGrote]

@JustinGrote it's not possible to run Docker on Windows GitHub runner currently.

Whoops you are right, derp. Maybe I can figure out a nested docker in ACI but I won't spend too much time on it for now.

[catthehacker]

@JustinGrote If we get newer Windows version on GHA (ref: actions/runner-images#3138), we might be able to have Docker in WSL2 and run tests.
If not, I'll most likely add self-hosted runner.

[cplee]

@JustinGrote - Adding needs-work tag due to conflicts

[JustinGrote]

Thanks, will rebase and sort it out.

[JustinGrote]

@cplee Squashed and rebased on master. Conflicts were parallel fixes to tests. Should be ready for re-review

[cplee]

Looks like this is causing MacOS tests to fail now :(

[catthehacker]

--- FAIL: TestRunContext_GetBindsAndMounts (0.00s)
    --- FAIL: TestRunContext_GetBindsAndMounts//mnt/linuxBind (0.00s)
    --- PASS: TestRunContext_GetBindsAndMounts//mnt/linux (0.00s)
    --- FAIL: TestRunContext_GetBindsAndMounts//mnt/path_with_spaces/linuxBind (0.00s)
    --- PASS: TestRunContext_GetBindsAndMounts//mnt/path_with_spaces/linux (0.00s)

[JustinGrote]

@cplee I'm working on the mac test now

[JustinGrote]

@cplee I'm working on the mac test now

Found it, bad test, forgot that MacOS adds that :delegated to the binds. Force pushing the update plus your latest master rebase

[JustinGrote]

@cplee test fixed but now currently failing on new linter tests, rebase it when those tests get fixed and it should be OK.

[catthehacker]

(JustinGrote/issue587)> ~/go/bin/golangci-lint run
pkg/runner/runner_test.go:104: File is not `goimports`-ed (goimports)
    {"testdata", "issue-597", "push", "", platforms, ""},
pkg/runner/run_context.go:554: unnecessary leading newline (whitespace)
func (ghc *githubContext) isLocalCheckout(step *model.Step) bool {

⏎                                           

[JustinGrote]

@catthehacker The imports one comes from master, I can fix the whitespace one however.
https://github.com/nektos/act/runs/2493880863

[JustinGrote]

@catthehacker @cplee just a reminder that the linting error is currently coming from master, not this PR, so I won't touch it assuming it will be fixed in master to avoid a conflict.

[JustinGrote]

@catthehacker @cplee ugh I just noticed that a maybeCopy was added in a recent commit I'll need to reconcile, will report back.

[catthehacker]

@catthehacker @cplee just a reminder that the linting error is currently coming from master, not this PR, so I won't touch it assuming it will be fixed in master to avoid a conflict.

it seems that lint issue is in your branch and in master it's already fixed

[JustinGrote]

@catthehacker right, I didn't see that new changes had landed, working on that but once I rebase those in it should be good.

[catthehacker]

looks good to me! if we miss anything, I'll be sure to push fix 😸

[JustinGrote]

Yay! Thank you @cplee and @catthehacker, I'll try to be available for any regressions that occur since this was a bit of a broad change, but this will lay the groundwork for some additional things as well.