Skip to content

feat: Host environment#1293

Merged
mergify[bot] merged 47 commits intomasterfrom
host_environment
Nov 16, 2022
Merged

feat: Host environment#1293
mergify[bot] merged 47 commits intomasterfrom
host_environment

Conversation

@ChristopherHX
Copy link
Contributor

@ChristopherHX ChristopherHX commented Aug 5, 2022 •
edited
Loading

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 •
edited
Loading

🦙 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 •
edited
Loading

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 mentioned this pull request Aug 5, 2022
Closed
@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

KnisterPeter commented Aug 6, 2022

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

@ChristopherHX ChristopherHX mentioned this pull request Aug 6, 2022
Closed
ChristopherHX
ChristopherHX commented Oct 19, 2022
View reviewed changes
pkg/container/host_environment.go
}
}

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

@ChristopherHX ChristopherHX Oct 19, 2022 •
edited
Loading

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 •
edited by ChristopherHX
Loading

@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
@ChristopherHX ChristopherHX mentioned this pull request Nov 11, 2022
Closed
cplee
cplee approved these changes Nov 16, 2022
View reviewed changes
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

ChristopherHX commented Nov 16, 2022

@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

ChristopherHX commented Nov 16, 2022

Thank you, it was a long way

@KnisterPeter
Copy link
Member

KnisterPeter commented Nov 16, 2022

Thanks for your hard work @ChristopherHX

@ChristopherHX ChristopherHX mentioned this pull request Nov 23, 2022
Closed
@ChristopherHX ChristopherHX mentioned this pull request Nov 30, 2022
Closed
20 tasks
@ChristopherHX ChristopherHX mentioned this pull request Feb 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cplee cplee cplee approved these changes

@catthehacker catthehacker Awaiting requested review from catthehacker

@ZauberNerd ZauberNerd Awaiting requested review from ZauberNerd

+1 more reviewer

@KnisterPeter KnisterPeter KnisterPeter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/XXL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@ChristopherHX @KnisterPeter @cplee @ZauberNerd @catthehacker