remove suite hook configuration and make information flow one-way #28090
Conversation
openshift-ci
bot
added
the
approved
|
/retest |
deads2k
force-pushed
the
invariant-08-suite-hook-madness
branch
from
63aed13
to
371a1e9
Compare
|
Job Failure Risk Analysis for sha: 371a1e9
|
deads2k
force-pushed
the
invariant-08-suite-hook-madness
branch
from
371a1e9
to
2b5e5e0
Compare
|
/retest |
|
/hold is missing 60 tests. The parallel suite and build suites look good. |
openshift-ci
bot
added
the
do-not-merge/hold
|
Job Failure Risk Analysis for sha: 2b5e5e0
|
do not merge until resolved. Interestingly, it appears in https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/28090/pull-ci-openshift-origin-master-e2e-gcp-ovn-upgrade/1683898197027590144 and doesn't appear in https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/28090/pull-ci-openshift-origin-master-e2e-gcp-ovn-rt-upgrade/1683898196247449600 |
|
OH, it didn't start the upgrade for RT
/retest |
| @@ -13,6 +12,10 @@ import ( | |||
| "syscall" | |||
| "time" | |||
|
|
|||
| "github.com/openshift/origin/pkg/test_suite_definition" | |||
There was a problem hiding this comment.
Via https://go.dev/blog/package-names
"Good package names are short and clear. They are lower case, with no under_scores or mixedCaps. They are often simple nouns"
I would suggest just "suites" here, or "testsuites".
|
|
||
| We describe test suites sufficiently for our suite runners and e2e monitor to understand | ||
| what setup, monitoring, and invariant tests should be enforced. | ||
| We have those react to this information instead of the other way around because we can know the |
There was a problem hiding this comment.
Good README, but "those" should be clarified here.
| This package contains the definition of all test suites. | ||
|
|
||
| We describe test suites sufficiently for our suite runners and e2e monitor to understand | ||
| what setup, monitoring, and invariant tests should be enforced. |
There was a problem hiding this comment.
Is "run" clearer than "enforced" here?
| @@ -0,0 +1,10 @@ | |||
| This package contains the definition of all test suites. | |||
|
|
|||
| We describe test suites sufficiently for our suite runners and e2e monitor to understand | |||
There was a problem hiding this comment.
Confused by e2e monitor here, vs "monitoring" in the subsequent part of the sentence, are they not the same thing?
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, dgoodwin 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 |
|
openstack didn't install. Not going to wait on that one, but /retest for history. |
|
plan to button merge on the correct number of tests in ovn-rt-upgrade. |
|
583 /hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
moving hooks out of the same package as the commands that run them allows us to have a starting point for a structured command with known inputs. the test suites become an input into the commands as opposed to the bi-directional dependency that we have today.
/assign @dgoodwin
openstack-test-openstackappears to be completely missing. No logs, no indication of failure. Suggest chasing after merge.numbers are against #27980 which adds tests, so this is looking great.