Probably fixes#431 (see investigation notes: https://code.forgejo.org/forgejo/runner/issues/431#issuecomment-50810) by extending the timeout for all cleanup actions from 1 minute to 30 minutes, and run cleanup after the task status is reported back to Forgejo. Cleanup logging now goes in the runner log rather than the job log, and it was tweaked to be less verbose and contain more useful identifying information on the job in case of error.
Manual testing confirms that Forgejo receives the success/failure status of the task before cleanup begins, that no further logging reaches the task at that point, and that the longer cleanup timeout is applied as expected.
This is an alternate, simplified version of #779.
<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- bug fixes
- [PR](https://code.forgejo.org/forgejo/runner/pulls/798): <!--number 798 --><!--line 0 --><!--description Zml4OiBsb25nZXIgdGltZW91dHMgb24gam9iIGNsZWFudXAgd2l0aG91dCB3YWl0aW5nICh2Mik=-->fix: longer timeouts on job cleanup without waiting (v2)<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/798
Reviewed-by: earl-warren <earl-warren@noreply.code.forgejo.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
they override any similar options from the configuration file since it would not make much sense to define a health check that applies to all containers, it is only ever meaningful for services.
```yaml
jobs:
mysql:
runs-on: ubuntu-latest
container: mysql:8
services:
maindb:
image: mysql:8
env:
MYSQL_DATABASE: dbname
MYSQL_USER: dbuser
MYSQL_PASSWORD: dbpass
MYSQL_RANDOM_ROOT_PASSWORD: yes
options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3
```
they are also allowed in job.<id>.container.options although they are not useful because they are harmless
See also the [associated documentation](https://codeberg.org/forgejo/docs/pulls/1366) pull request.
---
<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- features
- [PR](https://code.forgejo.org/forgejo/runner/pulls/784): <!--number 784 --><!--line 0 --><!--description ZmVhdDogLS1oZWFsdGgtKiBvcHRpb25zIGFyZSBhbGxvd2VkIGluIGpvYi48aWQ+LnNlcnZpY2VzLjxpZD4ub3B0aW9ucw==-->feat: --health-* options are allowed in job.<id>.services.<id>.options<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/784
Reviewed-by: Gusted <gusted@noreply.code.forgejo.org>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
Otherwise resources linger when tests are checking for conditions that
require a workflow to fail.
```sh
earl-warren:~/software/runner$ go test -count=1 -run='TestJobExecutorWorkflows/uses-github-short-sha' ./act/runner
ok code.forgejo.org/forgejo/runner/v9/act/runner 0.276s
earl-warren:~/software/runner$ docker network ls | grep WORKFLOW
2e57caeb514d WORKFLOW-cc93880718786fef12c18e53feecee5c8ef1cc155840bf4c0275a2fca61b99b9_JOB-test-test-network bridge local
earl-warren:~/software/runner$
```
<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- other
- [PR](https://code.forgejo.org/forgejo/runner/pulls/780): <!--number 780 --><!--line 0 --><!--description Y2hvcmUoY2kpOiBhdXRvcmVtb3ZlIHdoZW4gcnVubmluZyBhIHdvcmtmbG93IGluIGEgdGVzdA==-->chore(ci): autoremove when running a workflow in a test<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/780
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
It will be imported by Forgejo.
<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- other
- [PR](https://code.forgejo.org/forgejo/runner/pulls/777): <!--number 777 --><!--line 0 --><!--description Y2hvcmU6IHRvIGFsbG93IHRoZSBydW5uZXIgdG8gYmUgaW1wb3J0ZWQsIHY5IG5lZWRzIHRvIGJlIGluIHRoZSBnbyBtb2R1bGU=-->chore: to allow the runner to be imported, v9 needs to be in the go module<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/777
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
- use https://code.forgejo.org/forgejo/hello-world-docker-action/ that is a copy of https://github.com/actions/hello-world-docker-action/ with a patch to not use the docker hub
- node:16-buster-slim replaced with code.forgejo.org/oci/node:22 and checked that whatever is tested with it is not likely to be disturbed by the change in version
- change base images of Dockerfile to be from code.forgejo.org/oci/ equivalents
- use code.forgejo.org/forgejo/runner:8 instead of catthehacker/ubuntu:runner-latest
To test services:
- use code.forgejo.org/oci/bitnami/postgresql:16 instead of postgres:12
- use code.forgejo.org/oci/apache-git:1 instead of nginx:latest
Skip flaky test and [open an issue for it](https://code.forgejo.org/forgejo/runner/issues/763).
---
<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- bug fixes
- [PR](https://code.forgejo.org/forgejo/runner/pulls/762): <!--number 762 --><!--line 0 --><!--description Zml4KGNpKTogdXNlIGNvZGUuZm9yZ2Vqby5vcmcgaW5zdGVhZCBvZiB0aGUgZG9ja2VyIGh1Yg==-->fix(ci): use code.forgejo.org instead of the docker hub<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/762
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
keep it DRY
<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner/-->
- other
- [PR](https://code.forgejo.org/forgejo/runner//pulls/759): <!--number 759 --><!--line 0 --><!--description Y2hvcmU6IGEgc2luZ2xlIHRlc3R1dGlscyBmb3IgYm90aCBhY3QgYW5kIHRoZSBydW5uZXI=-->chore: a single testutils for both act and the runner<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/759
Reviewed-by: fnetX <fnetx@noreply.code.forgejo.org>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
- remove go vet and build as they are now redundant
- replace ./pkg with ./act
- rename the workflow from test to act to distinguish it from the
runner workflow
cleanup and factorization with the runner workflow is left for later
To prepare for a smooth merge in the runner codebase.
- run with --fix for gofumpt and golangci
- manual edits for
- disabling useless package naming warning
- rename variables that had underscore in their name
- remove trailing else at the end of a few functions
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/206
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
Only changes test files.
---
- remove tests specific to running on a host with no container
they are the same as with the containers (TestRunEventHostEnvironment)
- prefix the name of the tests with a distinctive name to
more easily run them together
- use code.forgejo.org/oci images whereever possible
- remove some tests that are either
- difficult to understand (ancient bugs)
- not yet well understood (related to reusable workflows)
- depend on github (generation of workflows to be run on the fly
e.g. updateTestIfWorkflow)
- fix the TestSanitizeNetworkAlias tests that were not run
and make them easier to debug
- disable tests of options that are forbidden in Forgejo Actions
(testdata/container-hostname/push.yml)
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/202
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
- the model defines them as strings and can parse them either as string or their effective type (boolean, number)
- add workflow validation when reading all testdata
- add fail-fast, max-parallel, timeout-minutes, cancel-timeout-minutes to test workflows in the jobparser tests
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/203
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
If not the schema validation will fail because it will be try to validate as if not calling a reusable workflow.
```
=== RUN TestWorkflowCallRunsOn
schema_test.go:119:
Error Trace: /home/earl-warren/software/act/pkg/schema/schema_test.go:119
Error: Received unexpected error:
Line: 10 Column 5: Failed to match job-factory: Line: 12 Column 5: Unknown Property uses
Line: 13 Column 5: Unknown Property with
Line: 15 Column 5: Unknown Property secrets
Line: 10 Column 5: Failed to match workflow-job: Line: 11 Column 5: Unknown Property runs-on
Test: TestWorkflowCallRunsOn
```
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/194
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
- prefer the lowercase version of the DNS name which is more common they are case insensitive anyway
- fix the inverted information when sanitation happens
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/197
Reviewed-by: msrd0 <msrd0@noreply.code.forgejo.org>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
An invalid workflow_{dispatch,call} key with a type that is not a map may attempt to use a nil map. It happens randomly as the order of the key maps is not guaranteed. Without this fix, the tests will fail 100% of the time with:
`go test -count=500 -run=TestParseRawOn/on:___workflow_ -v ./pkg/jobparser/`
Regression from https://code.forgejo.org/forgejo/act/pulls/45
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/193
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
For instance, the volume name derived from the workflow name may exceed the file system limit when the container name it is derived from is too long.
Fixesforgejo/runner#152
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/191
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
This created confusion when the interpretation of the absence of `runs-on` changed unexpectedly. Making it mandatory fixes this ambiguity.
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/192
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
- s/[^A-Z0-9-]/_/g
- add a log line in case the name is sanitized
Closesforgejo/runner#226
---
It is breaking because it will fail jobs that rely on service names that contain characters that are sanitized
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/190
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
In the current implementation, the network will only be created and be scheduled to be deleted when there is a service:
```go
func (rc *RunContext) networkName() (string, bool) {
if len(rc.Run.Job().Services) > 0 || rc.Config.ContainerNetworkMode == "" {
return fmt.Sprintf("%s-%s-network", rc.jobContainerName(), rc.Run.JobID), true
}
return string(rc.Config.ContainerNetworkMode), false
}
```
Therefore it does not currently make a difference. However, in case the network creation logic changes and a network is created even if a service is not present, it would be incorrect not to delete it.
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/189
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
This is a followup of https://code.forgejo.org/forgejo/act/pulls/170 so that it is possible to read a workflow without validation. It is not uncommon for Forgejo to read a workflow just to extract a few information from it, knowing it has been validated before. It would be a performance regression if schema validation happened in these cases.
This is a port of https://github.com/nektos/act/pull/2717/files
It is a breaking change in the context of Forgejo and Forgejo runner because it will need to add the new `validate` argument when reading workflows.
Co-authored-by: ChristopherHX <christopher.homberger@web.de>
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/180
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
- do not override username and password when looping over services
- split prepareJobContainer out of startJobContainer
- split getNetworkName out as it is used by both
- add unit tests for prepareJobContainer
- make containre.NewContainer mockable
- add MockVariable helper
Closesforgejo/runner#575
---
Note to reviewers: do not show whitespace change, the refactor will show in a minimal way. When the fix is reverted the tests fail as follows:
```
Diff:
--- Expected
+++ Actual
@@ -81,4 +81,4 @@
Image: (string) (len=10) "some:image",
- Username: (string) (len=17) "containerusername",
- Password: (string) (len=17) "containerpassword",
+ Username: (string) (len=16) "service2username",
+ Password: (string) (len=16) "service2password",
Entrypoint: ([]string) (len=3) {
Test: TestStartJobContainer/Overlapping
```
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/181
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
- rc.getToolCache(ctx) is used to figure out RUNNER_TOOL_CACHE and returns RUNNER_TOOL_CACHE if it is found in the runner config, e.g.
```yaml
container:
env:
RUNNER_TOOL_CACHE: /srv/toolcache
```
- store the value in the new `toolCache` data member for containers, in the same way it is done for host
- GetRunnerContext for containers return `toolCache` instead of a hard coded string
- add integration test
Closesforgejo/runner#184
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/178
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
It is a breaking change because it changes how the shell is
determined.
Before, if `jobs.<job_id>.container.image` is set and the shell is not
specified, it defaults to `sh` instead of `bash`.
After, regardless of `jobs.<job_id>.container.image`, if the shell is
not specified, it defaults to `bash` if available, otherwise it
defaults to `sh`.
Rework the shell integration tests:
- Remove container specific tests because the special behavior related
to shell being set differently when a container image is present is
removed
- Modify the defaults test case to verify the fallback logic
- Use container images from code.forgejo.org to escape rate limiting
in the CI
- Add the missing node test
- Use
container:
image: code.forgejo.org/oci/node:22-bookworm
instead of
container: code.forgejo.org/oci/node:22-bookworm
because it silently failed to run (with no exit code)
- Prefer `-z "${BASH}"` because `-z ${BASH+x}` reads obscure
Closesforgejo/runner#150
Otherwise act would reject such workflows right away
(cherry picked from commit efc95959aca96034426dcb93275eede5a3f47b83)
Conflicts:
pkg/schema/workflow_schema.json
trivial context conflict
- has_with had undetected structure error now failing the schema
- empty_step and has_secrets are invalid and caught by the schema
there no longer is a need for them
* fix: use non strict schema to allow some undefined behavior
* GitHub Actions doesn't use the newer strict schema in the service
* Tolerate more hallucinations
* Update workflow.go
* Update workflow.go
* Update pkg/model/workflow.go
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
---------
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit b52da6190c971733cdf3ffcef97cdbdce4ba42e5)
- log job result as info not as debug
- add test
---
v6.4.0 regression introduced in 4880b091a2
It did not fail a test because the [original fix](https://code.forgejo.org/forgejo/act/pulls/67/files) has tests only for the case where a step is skipped, not when a job is skipped.
Closesforgejo/runner#660
Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/172
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
