adding Gateway API conformance tests #60217
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lihongan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/pj-rehearse |
|
@lihongan: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
lihongan
force-pushed
the
add-gateway-conformance-test
branch
from
048819e to
a4c01aa
Compare
|
/pj-rehearse |
|
@lihongan: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
lihongan
force-pushed
the
add-gateway-conformance-test
branch
from
a4c01aa to
7057f42
Compare
|
/pj-rehearse |
|
@lihongan: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
lihongan
force-pushed
the
add-gateway-conformance-test
branch
from
7057f42 to
c48bf24
Compare
|
/pj-rehearse |
|
@lihongan: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
lihongan
force-pushed
the
add-gateway-conformance-test
branch
from
c48bf24 to
8fdbacc
Compare
|
/pj-rehearse |
|
@lihongan: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
lihongan
force-pushed
the
add-gateway-conformance-test
branch
from
8fdbacc to
00ad76a
Compare
|
/pj-rehearse |
|
@lihongan: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
lihongan
force-pushed
the
add-gateway-conformance-test
branch
from
00ad76a to
7fe2037
Compare
|
/pj-rehearse |
|
@lihongan: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
lihongan
force-pushed
the
add-gateway-conformance-test
branch
from
7fe2037 to
bad1a28
Compare
|
/pj-rehearse |
|
@lihongan: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
lihongan
force-pushed
the
add-gateway-conformance-test
branch
from
bad1a28 to
cd9209c
Compare
|
/pj-rehearse |
|
@lihongan: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@lihongan: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| echo "Go version: $(go version)" | ||
| cd /go/src/github.com | ||
| mkdir kubernetes-sigs && cd kubernetes-sigs |
There was a problem hiding this comment.
Using && here means that the cd command won't run if the directory already exists or if the mkdir command fails for any other reason:
$ mkdir foo && cd foo
$ pwd
/home/mmasters/foo
$ cd ..
$ mkdir foo && cd foo
mkdir: cannot create directory ‘foo’: File exists
$ pwd
/home/mmasters
$
mkdir -p doesn't return an error if the directory already exists:
$ ls -d foo
foo
$ mkdir -p foo && cd foo
$ pwd
/home/mmasters/foo
$
So it is better to use mkdir -p to avoid an error if the directory already exists (I assume we want to use the existing directory in this case), and use a separate cd command so that a failing mkdir command causes the script to exit on error:
| mkdir kubernetes-sigs && cd kubernetes-sigs | |
| mkdir -p kubernetes-sigs | |
| cd kubernetes-sigs |
| FEATURE_GATES: '[GatewayAPI=true]' | ||
| FEATURE_SET: CustomNoUpgrade |
There was a problem hiding this comment.
We will need to update this once openshift/api#2081 merges (which we want to happen as soon as possible).
There was a problem hiding this comment.
OK, once openshift/api#2081 merges we could just set FEATURE_SET: TechPreviewNoUpgrade.
And finally we will promote to GA in 4.19 right?
| # currently CRD v1.0.0 is supported | ||
| git clone --branch release-1.0 https://github.com/kubernetes-sigs/gateway-api |
There was a problem hiding this comment.
It is good that the CRD version is explicit. However, I wonder whether it would be better and practical to specify the version in the openshift/cluster-ingress-operator repository. We might eventually want to use this same job for multiple release branches with various CRD versions, and anyway, it would make version bumps easier if the CRD version for conformance tests and the CRD manifests were both in the same repository (namely openshift/cluster-ingress-operator).
If it isn't easy to read the CRD version from another repository, then we can leave this as a follow-up.
There was a problem hiding this comment.
I suppose the simplest thing would be to add a Makefile target in openshift/cluster-ingress-operator for running the conformance tests.
There was a problem hiding this comment.
Good suggestion, let me try if I can make it with Makefile.
| @@ -143,6 +143,18 @@ tests: | |||
| requests: | |||
| cpu: 100m | |||
| workflow: ipi-aws | |||
| - always_run: false | |||
There was a problem hiding this comment.
Shouldn't we remove the entry for e2e-aws-gatewayapi above? Doesn't this replace it?
There was a problem hiding this comment.
I don't think the conformance test can replace existing e2e-aws-gatewayapi, the conformance is to test the implementation matches the API specification, but implementation specific features will not be covered by it. For example, e2e-aws-gateway covers the OSSM and DNSRecord tests but conformance doesn't.
We could run conformance tests as part of our full e2e test suite. see https://gateway-api.sigs.k8s.io/concepts/conformance/#conformance
There was a problem hiding this comment.
Thanks. I misunderstood. Now I see the tests are completely different:
- ref: ingress-gatewayapi-conf-gatewayclass refers to ci-operator/step-registry/ingress/gatewayapi/conf-gatewayclass/ingress-gatewayapi-conf-gatewayclass-ref.yaml
- ref: ingress-gatewayapi-conformance-tests refers to ci-operator/step-registry/ingress/gatewayapi/conformance-tests/ingress-gatewayapi-conformance-tests-ref.yaml
This is a new pattern for us. Normally we make very few code additions here because we keep test code in the repo it is used in. I wonder what the benefit is to keep it in the step-registry?
Also, what will happen if the ingress-gateway-conf-gatewayclass fails - can we make this exit instead of running the next test that depends on it (ingress-gatewayapi-conformance-tests)?
If we keep the test code on the step-registry, please add lots of details in the commit message about what was needed in order to make this work.
Adding optional job for upstream gateway api conformance test