Cirrus CI runner: refactor #7940
Conversation
openshift-ci-robot
added
the
approved
edsantiago
force-pushed
the
runner_refactor
branch
from
74f68ee to
31533bf
Compare
| case "$testsuite" in | ||
| integration|system) output_filter=logformatter ;; | ||
| *) output_filter="cat" ;; | ||
| esac |
There was a problem hiding this comment.
Unnecessary: dotest() is only ever invoked for system or integration tests, nothing else.
There was a problem hiding this comment.
Correct, at the time I wasn't aware the logformatter worked with system-tests.
There was a problem hiding this comment.
(I was referring to my own mistake in #7924 -- I added the above case statement in dotest(), not realizing that the * condition would never be triggered).
contrib/cirrus/runner.sh
Outdated
| esac | ||
|
|
||
| # ginkgo doesn't play nicely with C Go (FIXME: what does this comment mean?) |
There was a problem hiding this comment.
@cevich can you illuminate this comment? If it's important, I'd like to reword it in such a way that it will help someone like me.
There was a problem hiding this comment.
I think this comment can be deleted, it was documenting something that IIRC was fixed in the Makefile instead.
contrib/cirrus/runner.sh
Outdated
| # This script is intended to be called by automation or humans, | ||
| # from a specially configured environment. Depending on the contents | ||
| # of various variable, entirely different operations will be performed. | ||
| # This script runs in the Cirrus CI environment, invoked from .cirrus.yml |
There was a problem hiding this comment.
Also (since I'm working on it) this and most of the scripts can be invoked by hand, assuming your inside a hack/get_ci_vm.sh environment. Maybe say, "...from .cirrus.yml or a specially constructed environment"?
There was a problem hiding this comment.
Fixed, but with a TBI.
contrib/cirrus/runner.sh
Outdated
| # most notably: | ||
| # | ||
| # PODBIN_NAME : "podman" (i.e. local) or "remote" | ||
| # CONTAINER : 1 to run in container, 0 otherwise |
There was a problem hiding this comment.
# CONTAINER : 1 currently running inside a container, 0 currently running on the host
# TEST_ENVIRON : 'host' or 'container'; desired environment for $TEST_FLAVOR testing
| *) | ||
| die "Unknown/Unsupported \$TEST_FLAVOR=$TEST_FLAVOR" ;; | ||
| esac | ||
| handler="_run_${TEST_FLAVOR}" |
There was a problem hiding this comment.
THANK YOU...this is SO much easier to read.
|
LGTM |
While reviewing containers#6784 I found myself having a lot of trouble with this script: it was a complicated mix of case statement and helper functions, requiring a reader to jump back and forth between the two. This PR defines a convention such that a given TEST_FLAVOR=foo must have a corresponding _run_foo() handler function. The goal is to have all TEST_FLAVOR-related code in one place, or at least less scattered (integration and system tests still rely on other helper functions). Signed-off-by: Ed Santiago <[email protected]>
edsantiago
force-pushed
the
runner_refactor
branch
from
31533bf to
e112555
Compare
|
/approve |
openshift-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
While reviewing #6784 I found myself having a lot of trouble
with this script: it was a complicated mix of case statement
and helper functions, requiring a reader to jump back and
forth between the two.
This PR defines a convention such that a given TEST_FLAVOR=foo
must have a corresponding _run_foo() handler function. The
goal is to have all TEST_FLAVOR-related code in one place,
or at least less scattered (integration and system tests
still rely on other helper functions).
Signed-off-by: Ed Santiago [email protected]