fix: evaluate jobs.<job_id>.container.volumes #1244
No reviewers
mfenniak
Labels
No labels
FreeBSD
Kind/Breaking
Kind/Bug
Kind/Chore
Kind/DependencyUpdate
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Windows
linux-powerpc64le
linux-riscv64
linux-s390x
run-end-to-end-tests
run-forgejo-tests
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/runner!1244
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "aahlenst/runner:container-volume-expressions"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #991
fix: evaluate jobs.<job_id>.container.volumesto WIP: fix: evaluate jobs.<job_id>.container.volumes@ -293,4 +293,0 @@{workdir, "job-container-non-root", "push", "", platforms, secrets},{workdir, "job-container-invalid-credentials", "push", "failed to handle credentials: failed to interpolate container.credentials.password", platforms, secrets},{workdir, "job-container-env-reference", "push", "", platforms, map[string]string{"ALPINE_TAG": "3.22"}},{workdir, "container-hostname", "push", "", platforms, secrets},Just peeking at your WIP in advance --container-hostnamemight have been unintentionally removed when you moved these.Ah, nevermind, I see it's moved into
job-container-options.I renamed the test because it's about options. Setting the host name is one of them. If you're not happy with the change, I can revert it.
a17f55939d3766ab5242WIP: fix: evaluate jobs.<job_id>.container.volumesto fix: evaluate jobs.<job_id>.container.volumesSo, ready to review.
The test failures caught me off guard. I wasn't aware that
make integration-testdoes not includego test -race -timeout 30m ./act/runner/.... Is that something we could add?On a side note: it would probably be a good idea if we ran the test suite on a system with SELinux. There are code paths in the runner that check for the presence of SELinux. Some tests are failing right now with SELinux enabled. Running the test suite with Podman (without being root) would probably be good, too. I'd be happy to assist.
@ -158,0 +157,4 @@interpolatedVolumes := make([]string, 0, len(container.Volumes))for _, volume := range container.Volumes {interpolatedVolumes = append(interpolatedVolumes, rc.ExprEval.Interpolate(ctx, volume))}The duplication is not great, but basically the entire method is almost a duplicate of
GetServiceBindsAndMounts(and there are more cases like that). However, I don't yet feel comfortable unifying them.@aahlenst wrote in #1244 (comment):
I think this is a legacy of
actbeing a separate repo recently, which was smashed together into the runner due to the high interdependence. Some work was done to unify the makefile, but some stuff was copied over into the CI workflows instead.The
./act/runner/...tests are run in two docker environments in the CI with a matrix build which I'd want to preserve, so merging them would be increasing test resource utilization a bit by taking the 5mmake integration-testand doing it twice. But I think it's a minor detail and possibly even one that is justified as a good integration test. So, I'd be 👍 on merging them and eliminating this hidden detail.Adding Podman would be great, for sure. After the second Docker environment was added recently, adding a Podman test run was on my mind but I didn't complete it and I'm not planning to immediately get back to it, but I'd be happy to review it.
SELinux is an area where I know very little about, and I wasn't aware of that specialized case.
cascading-pr updated at actions/setup-forgejo#801
Thanks for the contribution! 🙂