e2e refactor - remove the need to use yamls for testing #380
Conversation
stlaz
added
the
sig-auth-acceptance
stlaz
force-pushed
the
e2e-rework
branch
3 times, most recently
from
6880e0f to
132f339
Compare
| return 200 'metrics endpoint reached\n'; | ||
| } | ||
| } | ||
| ` |
There was a problem hiding this comment.
What I don't like the most is that we have a nginx config as string here and in identityheaders/{secure,default}/configmap-nginx.yaml. I prefer to have them as a dedicated file and NOT as string in here.
In a perfect world upstream and potentially the client would have their own builders as well, with this nice file-reading like with the templates, but I think this would be too far-stretched... yet.
There was a problem hiding this comment.
I agree it's not ideal. If we'd like an nginx config builder, we can do it in a follow-up.
For now I like that you don't need to look in a different file to see this simple, default config.
| verb: get | ||
| path: /metrics | ||
| ` | ||
|
|
There was a problem hiding this comment.
Here, I am a little less strongly against it, but I would prefer to have it in a dedicated file.
There was a problem hiding this comment.
Being able to see the config right away in the same file seems valuable to me.
| AddUserClusterRoleBinding("test-client", testtemplates.GetMetricsRoleForClient()). | ||
| WithClientCerts("test-client"). | ||
| UpdateFlags(map[string]string{"client-ca-file": "/var/run/configMaps/test-client-trust/ca.crt"}) | ||
| } |
There was a problem hiding this comment.
You want to ensure to not reuse certs?
| limitations under the License. | ||
| */ | ||
|
|
||
| package testtemplates |
There was a problem hiding this comment.
I really really like this file. We could actually use this to read all the files that we have, not just the templates 😄
| limitations under the License. | ||
| */ | ||
|
|
||
| package kubetest |
There was a problem hiding this comment.
Some of the lines in this file are very very long.
| return c | ||
| } | ||
|
|
||
| func (c *KRPTestConfig) Launch(client kubernetes.Interface) Action { |
There was a problem hiding this comment.
Uf, this is rather long and mostly the same pattern:
- create resource,
- add to deployment,
- and defer cleanup.
Not sure though if it makes sense to rework this as this PR is already very big.
There was a problem hiding this comment.
I agree it's a bit unfortunate. As I was writing this, I realized that this would probably have to be that one function that everybody hates but just deals with it.
We can try to do something with it in a follow-up.
As this PR shows, we really should wire context through in many places through the tests, perhaps that can be done along with it.
Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
Signed-off-by: Stanislav Láznička <[email protected]>
|
/lgtm lets merge as is |
2 participants
The e2e tests in the current form are super hard to review whenever there are some new moves.
A huge part of why this is so hard is the super impractical use of YAML files for each test.
This PR creates a new
kubetest.Actionthat allows moving the test config into the test file ratherthan having to follow filenames and trying to eye where one YAML diffs from another.
I am building this PR on top sig-auth-acceptance branch but will cherry-pick it once (if) it merges.