Draft: Adds services capabilities #638

Markcial · 2021-04-26T10:05:30Z

Fixes #173

Hi, this is a draft MR. Will need some more work

The network is created and removed by name, causing collisions, and it is created only once, I am not sure if that is the right setup, maybe it might be needed to be different on each job.

Containers and services should be included on that network, it's done here but maybe there is a better way. This MR also lacks proper testing, some advice on this would be good.

The configuration on the services are done in a hacky way, they should have properly merged environment settings, and some other configuration values.

I can help on any further changes or if you feel that someone else can't take better care of it feel free to modify it.

Thanks for this project

catthehacker

Anything else looks good to me!

catthehacker · 2021-05-03T01:23:21Z

pkg/runner/run_context.go

+const defaultNetwork = "act_github_actions_network"
+
+func (rc *RunContext) createNetwork() common.Executor {
+	return func(ctx context.Context) error {
+		return container.NewDockerNetworkCreateExecutor(defaultNetwork)(ctx)
+	}
+}
+
+func (rc *RunContext) removeNetwork() common.Executor {
+	return func(ctx context.Context) error {
+		return container.NewDockerNetworkRemoveExecutor(defaultNetwork)(ctx)
+	}
+}
+


If another workflow references same services, there will be a clash.
Perhaps network should be named act-<workflow-name>-network

catthehacker · 2021-05-03T01:24:07Z

pkg/runner/run_context.go

+				c.Pull(false),
+				c.Create(),
+				c.Start(false),
+				c.ConnectToNetwork(defaultNetwork),


Network name clash.

catthehacker · 2021-05-03T01:24:37Z

pkg/runner/run_context.go

 			rc.JobContainer.Create(),
 			rc.JobContainer.Start(false),
+			rc.JobContainer.ConnectToNetwork(defaultNetwork),


Network name clash.

catthehacker · 2021-05-04T08:39:35Z

pkg/container/docker_network.go

 		if err != nil {
 			return err
 		}
+		fmt.Printf("%#v", network.ID)


Please use log instead of fmt, we should limit fmt only to formatting strings

Oh sorry, it was a rogue line, my apologies

pkg/runner/run_context.go

pkg/runner/runner_test.go

Co-authored-by: Ryan (hackercat) <me@hackerc.at>

pkg/runner/runner_test.go

catthehacker · 2021-05-04T09:28:59Z

It seems that it created 5 different act-test-network networks

[basic/build]   🐳  docker pull node:12.20.1-buster-slim
time="2021-05-04T09:27:22Z" level=debug msg="Image exists? true"
[basic/build] Removed container: 6648844bde9265313d5fed13c0dbeaded7745c5deb5db238bb8ee514bf595278
[basic/build]   🐳  docker volume rm act-basic-build
[basic/build]   🐳  docker create image=node:12.20.1-buster-slim platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[basic/build] Created container name=act-basic-build id=d2af12ba6bb5dee3e5133bf1c8690d62ffc99b46397813da006a6b0d056e5570 from image node:12.20.1-buster-slim (platform: )
[basic/build] ENV ==> [RUNNER_TOOL_CACHE=/opt/hostedtoolcache RUNNER_OS=Linux RUNNER_TEMP=/tmp]
[basic/build]   🐳  docker run image=node:12.20.1-buster-slim platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[basic/build] Starting container: d2af12ba6bb5dee3e5133bf1c8690d62ffc99b46397813da006a6b0d056e5570
[basic/build] Started container: d2af12ba6bb5dee3e5133bf1c8690d62ffc99b46397813da006a6b0d056e5570
[basic/build]   🐳  docker network connect act-build-network act-basic-build
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
--- FAIL: TestRunEventPullRequest (0.80s)
    runner_test.go:226: assertion failed: error is not nil: Error response from daemon: network act-build-network is ambiguous (5 matches found on name): pull-request

Markcial · 2021-05-04T09:31:19Z

It seems that it created 5 different act-test-network networks

[basic/build]   🐳  docker pull node:12.20.1-buster-slim
time="2021-05-04T09:27:22Z" level=debug msg="Image exists? true"
[basic/build] Removed container: 6648844bde9265313d5fed13c0dbeaded7745c5deb5db238bb8ee514bf595278
[basic/build]   🐳  docker volume rm act-basic-build
[basic/build]   🐳  docker create image=node:12.20.1-buster-slim platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[basic/build] Created container name=act-basic-build id=d2af12ba6bb5dee3e5133bf1c8690d62ffc99b46397813da006a6b0d056e5570 from image node:12.20.1-buster-slim (platform: )
[basic/build] ENV ==> [RUNNER_TOOL_CACHE=/opt/hostedtoolcache RUNNER_OS=Linux RUNNER_TEMP=/tmp]
[basic/build]   🐳  docker run image=node:12.20.1-buster-slim platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[basic/build] Starting container: d2af12ba6bb5dee3e5133bf1c8690d62ffc99b46397813da006a6b0d056e5570
[basic/build] Started container: d2af12ba6bb5dee3e5133bf1c8690d62ffc99b46397813da006a6b0d056e5570
[basic/build]   🐳  docker network connect act-build-network act-basic-build
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
time="2021-05-04T09:27:23Z" level=debug msg="Error response from daemon: network act-build-network is ambiguous (5 matches found on name)"
--- FAIL: TestRunEventPullRequest (0.80s)
    runner_test.go:226: assertion failed: error is not nil: Error response from daemon: network act-build-network is ambiguous (5 matches found on name): pull-request

Yes, this was one of my concerns, I was investigating on how to have the network ID, because do the operations just with the network name is a bit flaky.

I have a busy week, but if you can wait until next monday I can dig on this

catthehacker · 2021-05-04T17:06:25Z

I kind of resolved it but then stuck with issue of too much IP ranges allocated

mergify · 2021-05-04T21:58:10Z

@Markcial this pull request is now in conflict 😩

mergify · 2021-05-04T21:58:12Z

@Markcial this pull request has failed checks 🛠

catthehacker · 2021-05-04T22:01:35Z

thank you bot, I know that -.-

Markcial · 2021-05-05T07:16:12Z

If you get something let me know, as I can't get to it before next monday.

I have an idea about using the network identifier to keep track on the networks and how to specifically remove them. But until I get some spare time I won't be able to try it.

Thanks for getting some time into it

Co-authored-by: Ryan (hackercat) <me@hackerc.at>

catthehacker · 2021-05-05T07:55:48Z

https://github.com/catthehacker/act-fork/tree/ticket-173
There is a lot in there unrelated changes as well (since I've a bad habit of going on refactoring spree when looking through code)

Markcial · 2021-05-05T07:59:25Z

https://github.com/catthehacker/act-fork/tree/ticket-173
There is a lot in there unrelated changes as well (since I've a bad habit of going on refactoring spree when looking through code)

Well don't worry, at least it's something to check for ideas, thanks

github-actions · 2021-06-05T00:17:55Z

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

Draft: Adds services capabilities

7d86160

pull-request-size bot added the size/L label Apr 26, 2021

catthehacker self-requested a review April 26, 2021 13:17

catthehacker requested changes May 3, 2021

View reviewed changes

cplee added the needs-work Extra attention is needed label May 3, 2021

Markcial added 2 commits May 4, 2021 10:22

Applies review changes

c96e1ea

Merge branch 'master' into ticket-173

147192d

catthehacker requested changes May 4, 2021

View reviewed changes

Removes unnecessary log line

ab55ab7

catthehacker requested changes May 4, 2021

View reviewed changes

pkg/runner/run_context.go Outdated Show resolved Hide resolved

Removes old code induced because of the rebase

13b4b18

catthehacker requested changes May 4, 2021

View reviewed changes

pkg/runner/runner_test.go Outdated Show resolved Hide resolved

Update pkg/runner/runner_test.go

d550c8c

Co-authored-by: Ryan (hackercat) <me@hackerc.at>

catthehacker requested changes May 4, 2021

View reviewed changes

pkg/runner/runner_test.go Outdated Show resolved Hide resolved

mergify bot added the conflict PR has conflicts label May 4, 2021

Markcial requested a review from a team as a code owner May 5, 2021 07:16

Update pkg/runner/runner_test.go

fb7339e

Co-authored-by: Ryan (hackercat) <me@hackerc.at>

github-actions bot added the stale label Jun 5, 2021

catthehacker self-assigned this Jun 5, 2021

github-actions bot closed this Jun 20, 2021

catthehacker mentioned this pull request Jul 14, 2021

Services not working #173

Closed

catthehacker mentioned this pull request Aug 11, 2021

Add support for GHA services #775

Closed

3 tasks

ChristopherHX mentioned this pull request Sep 15, 2022

Installation fails: "Failure - Main Use Node.js ${{ matrix.node-version }}" #1342

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Adds services capabilities #638

Draft: Adds services capabilities #638

Markcial commented Apr 26, 2021 •

edited by catthehacker

Loading

catthehacker left a comment

catthehacker May 3, 2021

catthehacker May 3, 2021

catthehacker May 3, 2021

catthehacker May 4, 2021

Markcial May 4, 2021

catthehacker commented May 4, 2021

Markcial commented May 4, 2021

catthehacker commented May 4, 2021

mergify bot commented May 4, 2021

mergify bot commented May 4, 2021

catthehacker commented May 4, 2021

Markcial commented May 5, 2021

catthehacker commented May 5, 2021

Markcial commented May 5, 2021

github-actions bot commented Jun 5, 2021

Draft: Adds services capabilities #638

Draft: Adds services capabilities #638

Conversation

Markcial commented Apr 26, 2021 • edited by catthehacker Loading

catthehacker left a comment

Choose a reason for hiding this comment

catthehacker May 3, 2021

Choose a reason for hiding this comment

catthehacker May 3, 2021

Choose a reason for hiding this comment

catthehacker May 3, 2021

Choose a reason for hiding this comment

catthehacker May 4, 2021

Choose a reason for hiding this comment

Markcial May 4, 2021

Choose a reason for hiding this comment

catthehacker commented May 4, 2021

Markcial commented May 4, 2021

catthehacker commented May 4, 2021

mergify bot commented May 4, 2021

mergify bot commented May 4, 2021

catthehacker commented May 4, 2021

Markcial commented May 5, 2021

catthehacker commented May 5, 2021

Markcial commented May 5, 2021

github-actions bot commented Jun 5, 2021

Markcial commented Apr 26, 2021 •

edited by catthehacker

Loading