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: Host environment #1293

Merged
merged 47 commits into from
Nov 16, 2022
Merged

feat: Host environment #1293

merged 47 commits into from
Nov 16, 2022

Conversation

ChristopherHX
Copy link
Contributor

@ChristopherHX ChristopherHX commented Aug 5, 2022

This is my refactored implementation of a non docker based executor

To test running actions without docker specify -P ubuntu-latest=-self-hosted

I refactored a larger amount of code

  • Allow to alter /var/run/act
    • otherwise concurrent executors on the same host conflict each other
    • this folder cannot be created on windows
  • Tweak PATH manipulation to use custom list seperator, while not running in docker
  • Different runner context if running on host vs. in a container

Fixes #97

I need to look into possible linting violations

Known Issues

  • windows cmd not working, omitted here to shrink size
  • Possible escaping problems for default shell, omitted here to shrink size
  • On windows cmdline needs to be added to the container interface, omitted here to shrink size
  • Default shell bash not suitable on windows
  • docker actions are intentionally failing, since window and macos runner doesn't support them officially and some platforms like freebsd, etc. doesn't support docker

I want to know if this design change has a chance to get merged

Some implementation Details

  • lookpath is mostly copied from the go stdlib, because the original didn't allow to provide a custom PATH variable for lookup and I want to make shure multiple parallel workers don't update PATH env var to use the stdlib directly
  • Most changes are done by replace operations, API Change
  • New if conditions to avoid a crash in Tests, these issues don't appear with a well constructed run_context

Commit Message

{{title}} (#1293)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 36 0 0.08s
✅ REPOSITORY gitleaks yes no 2.34s
✅ REPOSITORY git_diff yes no 0.0s
✅ REPOSITORY secretlint yes no 0.99s
✅ YAML prettier 1 0 0.48s
✅ YAML v8r 1 0 2.26s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #1293 (399f87b) into master (4f8da0a) will increase coverage by 2.86%.
The diff coverage is 66.63%.

@@            Coverage Diff             @@
##           master    #1293      +/-   ##
==========================================
+ Coverage   57.50%   60.37%   +2.86%     
==========================================
  Files          32       44      +12     
  Lines        4594     6950    +2356     
==========================================
+ Hits         2642     4196    +1554     
- Misses       1729     2451     +722     
- Partials      223      303      +80     
Impacted Files Coverage Δ
pkg/common/file.go 0.00% <0.00%> (ø)
pkg/container/host_environment.go 0.00% <0.00%> (ø)
pkg/container/util.go 0.00% <0.00%> (ø)
pkg/model/action.go 0.00% <0.00%> (ø)
pkg/model/step_result.go 0.00% <ø> (ø)
pkg/container/docker_run.go 12.82% <11.53%> (+7.27%) ⬆️
pkg/model/workflow.go 45.65% <23.80%> (-5.26%) ⬇️
...ontainer/linux_container_environment_extensions.go 24.32% <24.32%> (ø)
pkg/container/docker_pull.go 33.33% <33.33%> (ø)
pkg/container/file_collector.go 39.68% <39.68%> (ø)
... and 36 more

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

@ChristopherHX ChristopherHX marked this pull request as ready for review August 6, 2022 21:21
@ChristopherHX ChristopherHX requested a review from a team as a code owner August 6, 2022 21:21
@KnisterPeter
Copy link
Member

I will look into this starting next week.
It's a lot to review. But I like the idea

}
}

func (e *HostEnvironment) UpdateFromEnv(srcPath string, env *map[string]string) common.Executor {
Copy link
Contributor Author

@ChristopherHX ChristopherHX Oct 19, 2022

Choose a reason for hiding this comment

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

Until Today this was a copy from docker_run.go.
The copied code has big flaws with parsing the env file, so this code is rewritten from scatch to pass all existing tests.

Update:
See the original code https://github.com/actions/runner/blob/3fc993da5946feac15c8020c414bca450f018f3e/src/Runner.Worker/FileCommandManager.cs#L285-L367, which differs significantly from the current docker_run impl.

@mergify mergify bot removed the needs-work Extra attention is needed label Oct 19, 2022
@mergify mergify bot requested a review from a team October 19, 2022 20:56
@mergify
Copy link
Contributor

mergify bot commented Oct 24, 2022

@ChristopherHX this pull request is now in conflict 😩

Will be resolved once cplee finishs his first review.

@mergify mergify bot added the conflict PR has conflicts label Oct 24, 2022
@mergify mergify bot removed the conflict PR has conflicts label Nov 8, 2022
Copy link
Contributor

@cplee cplee left a comment

Choose a reason for hiding this comment

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

@ChristopherHX - sorry for the delays with this one. Finally got a chance to try out locally. Looks great, thanks for your patience!!

@ChristopherHX
Copy link
Contributor Author

@cplee I need your approval later a second time for this PR, because this PR is not the first in the merge queue we will see a small merge conflict today.

@cplee
Copy link
Contributor

cplee commented Nov 16, 2022

@ChristopherHX - I'll be watching 👀

@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2022

@ChristopherHX this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Nov 16, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2022

@ChristopherHX this pull request has failed checks 🛠

2 similar comments
@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2022

@ChristopherHX this pull request has failed checks 🛠

@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2022

@ChristopherHX this pull request has failed checks 🛠

@mergify mergify bot removed the needs-work Extra attention is needed label Nov 16, 2022
@mergify mergify bot merged commit f2b98ed into master Nov 16, 2022
@mergify mergify bot deleted the host_environment branch November 16, 2022 21:29
@cplee
Copy link
Contributor

cplee commented Nov 16, 2022

🎉🎉🎉

@ChristopherHX
Copy link
Contributor Author

Thank you, it was a long way

@KnisterPeter
Copy link
Member

Thanks for your hard work @ChristopherHX

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.

Add support for --no-container to run windows/macos
5 participants