Create and use a test entrypoint in templatized e2e #1799
Conversation
openshift-ci-robot
added
size/L
stevekuznetsov
left a comment
stevekuznetsov
left a comment
There was a problem hiding this comment.
I think we need the $TEST_COMMAND as a parameter env, as well as a container env, and pass it through three (?) times to get it to work?
@smarterclayton woo! template ux ++
| - name: TEST_SKIP | ||
| value: (\\[local\\]|\\[templates\\]|\\[Feature:Router\\]|Kubectl logs) | ||
| - name: TEST_COMMAND | ||
| value: TEST_FOCUS=Suite:openshift/conformance/parallel/minimal TEST_SKIP='(\\[local\\]|\\[templates\\]|\\[Feature:Router\\]|Kubectl |
There was a problem hiding this comment.
This has double escapes whereas the others have single? Is it a bug in the focus?
There was a problem hiding this comment.
- What format does
ginkgoexpect? - There is no semantic change here.
- If it worked before, it will still work. If it didn't, it won't.
- /shrug?
There was a problem hiding this comment.
yeah agreed on all counts
There was a problem hiding this comment.
I think \\[ is correct -- we want a literal \[ going to the Ginkgo regex, so whoever is using \[ is wrong.
There was a problem hiding this comment.
(out of band from this PR)
| env: | ||
| - name: HOME | ||
| value: /tmp/home | ||
| - name: TEST_COMMAND |
There was a problem hiding this comment.
Don't we still want this?
There was a problem hiding this comment.
AFAICT, the only reason for having it as an environment variable before was for this test (repeated in a few templates), which would be problematic if it were done by template substitution because of quotes:
release/ci-operator/templates/cluster-launch-e2e-upgrade.yaml
Lines 152 to 156 in 081c32a
We don't perform that test anymore, so that's not necessary. The only place that variable appears in the templates now is here:
That's a template substitution, the variable will be replaced by whatever value it has in the pod spec and quotes etc. will be preserved. I tested the instantiation with oc process with a few problematic values and they all worked correctly.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bbguimaraes, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@bbguimaraes: Updated the following 9 configmaps:
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| -test.timeout=20m ${PROVIDER_ARGS-} || rc=$? | ||
| fi | ||
| exit ${rc:-0} | ||
| function run-tests() { |
There was a problem hiding this comment.
We lost path here which broke installer. I'll open a PR
There was a problem hiding this comment.
We lost path here which broke installer. I'll open a PR
There was a problem hiding this comment.
I'm going to clean up the inconsistency and restore the fix in #1814
6 participants
Resurrecting #1527.
/assign @smarterclayton @stevekuznetsov