ci: Remove dead code, unused inputs and honor input refs - #941
Conversation
- Delete unused typos.toml (spell-check runs cspell, not typos). - Remove useless "Get revision SHA" steps and inline github.ref_name where the branch value is still needed (build-latest). - Remove the dead "Get image digest" and "Set short SHA" steps in build-latest. - Drop unused workflow inputs (runner, runner-arch-map, unused runner-archs, dead ref) and their call-site passes - Use the declared ref input into checkout (lint, unit_test, vm_test, kind_test) - Remove unused GIT_CLONE_PAT secret and no-op workflow_dispatch triggers that cannot run standalone (upload_s3, vm_test, kind_test) - Drop redundant host dependency install in kind_test (used only inside the node) Signed-off-by: Charalampos Mainas <charalampos.mainas@gmail.com>
✅ Deploy Preview for urunc canceled.
|
ab7669a to
e016a36
Compare
e016a36 to
bafe00f
Compare
We currently execute the end-to-end tests using the base ref of a PR in order to ensure that the PR's code works as expected and nothing in the PR trickes the tests in order to succeed. However, we also need to tests the changes that take place in the e2e tests. For that reason, another run of vm_tests is triggered using the PR's branch ref to check if the new code of end-to-end tests is working properly. Signed-off-by: Charalampos Mainas <charalampos.mainas@gmail.com>
bafe00f to
225a611
Compare
gntouts
left a comment
There was a problem hiding this comment.
Good job, the fixes look good. One addition I'd like to see:
The tests/ filter should also cover the other files the e2e job reads from
the checkout. The job consumes four things: tests/e2e (compiled),
script/dm_create.sh (vm_test.yml:146-148, runs as root), the root Makefile
(make prepare at :329, make <test> at :355/357), and go.mod/go.sum via
make prepare. The filter only tracks the first one.
Possible failures if we leave it:
- A PR that changes
dm_create.shmerges without ever running it. If it is
broken, every later PR fails at "Setup devmapper" across the whole matrix,
and the blame lands on the wrong commit. - A PR that renames a test target and updates the matrix in
vm_test.yml
fails withNo rule to make target: the workflow file resolves at the PR
head, but the checkout is base, so the new matrix runs against the old
Makefile. - A
TEST_OPTSchange (e.g. raising the 20m timeout) silently does not apply
at PR time.
The nightly runs main and catches all of this eventually, but that moves
detection to after the merge, which is what this gate exists to prevent.
Suggestion:
// Must track what vm_test.yml reads from the checkout.
const roots = ['tests/', 'Makefile', 'script/'];
const changed = files.some(f => roots.some(r => f.filename.startsWith(r)));
go.mod/go.sum matter less: the binary arrives prebuilt, and base tests
against base go.mod is already a consistent pair. Include them if you want
go mod verify exercised; either way is fine.This only widens the trigger for vm_test_head, the base-ref vm_test job is unchanged.
|
Thank you @gntouts for the review. I wanted to avoid I will also include the rest of the paths you mention and I will open a new issue to improve the e2e testing times and scope to better utilize the nightly testing and also do not stall CI in the PRs. |
Description
Edit: A new job has been added which executes the e2e vm_test when something changes in e2e using the PR's branch ref to checkout the code. Otherwise the base branch of the PR is used as a checkout ref, keeping the base testing code stable across new PRs.
Related issues
How was this tested?
In PR's CI
LLM usage
Assisted by Opus 4.8
Checklist
make lint).make test_ctr,make test_nerdctl,make test_docker,make test_crictl).