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

Support for docker steps in host environment #1674

Merged
merged 2 commits into from
Mar 14, 2023

Conversation

shubham149
Copy link
Contributor

Network mode in host environment is set to container network earlier. But the container network is not present. This PR uses default as network mode in case host environment is enabled.

Also, setting working directory for container step to same as one specified in the input params.

@shubham149 shubham149 requested a review from a team as a code owner March 10, 2023 13:19
@ChristopherHX
Copy link
Contributor

Also, setting working directory for container step to same as one specified in the input params.

I'm concerned about this change, ToContainerPath is used to move fileio from readonly checkout into a scratch space under .cache. This breaks file operations of host environment if the exact workdir should be translated

Seems like my tests also show this is a bad idea to remove.

  • you need a bind mount for workdir of host environment
  • you need a bind mount for env files (for outputs)

I'm 100% fine with the rest of your changes.

@mergify
Copy link
Contributor

mergify bot commented Mar 10, 2023

@shubham149 this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Mar 10, 2023
@shubham149
Copy link
Contributor Author

@ChristopherHX I have reverted workdir changes from this pull request.

Is there a way we can set the working directory for container steps same as input working directory for host env?

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #1674 (8be7541) into master (4989f44) will increase coverage by 1.51%.
The diff coverage is 73.57%.

@@            Coverage Diff             @@
##           master    #1674      +/-   ##
==========================================
+ Coverage   61.22%   62.73%   +1.51%     
==========================================
  Files          46       48       +2     
  Lines        7141     7463     +322     
==========================================
+ Hits         4372     4682     +310     
+ Misses       2462     2461       -1     
- Partials      307      320      +13     
Impacted Files Coverage Δ
pkg/container/docker_cli.go 82.23% <ø> (ø)
pkg/container/docker_logger.go 52.08% <ø> (ø)
pkg/container/docker_pull.go 33.33% <ø> (ø)
pkg/container/docker_run.go 14.08% <0.00%> (+0.49%) ⬆️
pkg/container/docker_volume.go 0.00% <ø> (ø)
pkg/container/file_collector.go 37.30% <0.00%> (ø)
pkg/container/host_environment.go 0.00% <0.00%> (ø)
pkg/exprparser/functions.go 66.32% <0.00%> (-1.04%) ⬇️
pkg/model/workflow.go 42.11% <ø> (ø)
pkg/model/planner.go 44.71% <21.27%> (-4.12%) ⬇️
... and 20 more

📣 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 Mar 10, 2023
@ChristopherHX
Copy link
Contributor

ChristopherHX commented Mar 10, 2023

You have started to work on a very complex area of act

The current design of HostEnvironment is not really compatible with docker actions..., multiple container path translations are not implemented and volume mounts of workdir won't work with stepcontainer without jobcontainer.

You can use tocontainerpath of

func (*LinuxContainerEnvironmentExtensions) ToContainerPath(path string) string {

Like used in the startjobcontainer implementation.

To translate the workdir for the docker action start, there are probably more places causing issues.

After the step container is created, you may need to replace some calls of jobcontainer.tocontainerpath with stepcontainer.tocontainerpath. (eventually you have to replace interface Container with ExecutionContext to get access)

Addidionally you need to translate all host paths of your inputs and env's back to a docker container paths. (Currently in act, either every env uses docker paths or host paths, never both at same time in one job)

@shubham149
Copy link
Contributor Author

shubham149 commented Mar 10, 2023

@ChristopherHX Can we merge this PR? I have removed the workDir changes from it. If anything is pending, let me know about it.
I will work on workDir change separately.

Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

Yes, you need another approval.

@mergify mergify bot merged commit 09de42f into nektos:master Mar 14, 2023
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.

3 participants