Use single vault instance in e2e tests#5950
Use single vault instance in e2e tests#5950jetstack-bot merged 4 commits intocert-manager:masterfrom
Conversation
cee4f2f to
fa83db5
Compare
0df1a11 to
e39d39b
Compare
58b081e to
087de80
Compare
|
Awesome initiative. Running helm install for every single Vault test is a massive waste of time. |
6810e17 to
1abbb20
Compare
04d1aad to
9b6290b
Compare
|
/test pull-cert-manager-master-e2e-v1-26 |
|
@irbekrm there was still one remaining bug. I fixed it in the last commit. |
768f3ef to
512b7ac
Compare
irbekrm
left a comment
There was a problem hiding this comment.
Thank you Tim, I have read through part of the code.
I would be interested to see some data as to how much improvement this PR actually makes.
At the moment after looking at the code I think that having a shared instance adds too much complexity for the potential gain and makes the already too complicated e2e test setup even more complicated- I am worried that because of that the code will not be maintainable and also we could end up not testing some of the Vault setup methods because of it being hard to reason about what the code does.
test/e2e/framework/addon/globals.go
Outdated
| if !g.SupportsGlobal() { | ||
| return nil, fmt.Errorf("requested global plugin does not support shared mode with current configuration") | ||
| } | ||
| toBeTransferred[idx] = data |
There was a problem hiding this comment.
Can we make []interface{} into map[AddonName]interface{} and use addon name as a key? (Add a Name method to addons)
There was a problem hiding this comment.
I think it is easier to keep this as a statically allocated array, otherwise we have to add logic for keys which are not that useful.
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
512b7ac to
d3ba8be
Compare
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
|
/test pull-cert-manager-master-e2e-v1-27 |
irbekrm
left a comment
There was a problem hiding this comment.
Thanks for making the changes Tim, it's a lot clearer.
I had another read through this and through Ginkgo docs this morning.
Can we not keep the more standard structure and make it so that we use the Ginkgo's existing mechanism for co-ordinating between first and all processes by putting all the code that needs to run just once into the first function argument and making all the setup code in the second function arg such that it can run on any process (as we have it currently)?
See the example in https://onsi.github.io/ginkgo/#parallel-suite-setup-and-cleanup-synchronizedbeforesuite-and-synchronizedaftersuite:~:text=We%20can%20use%20this%20behavior%20to%20set%20up%20shared%20external%20resources%20like%20so%3A
Or, is this not possible? I think this would be preferable instead of building our own mechanism (the isGinkgoProcessNumberOne variable) on top of what Ginkgo already has.
If this is not possible due to addons structure, happy to approve this PR as is, but perhaps add a note as to why or if it could be done in future by changing the addons structure.
The reason that we use |
Thanks for the reply. Yeah I don't think a boolean would be better. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inteon, irbekrm 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 |
Goal: reduce e2e test time & reduce flakes
NOTE: I removed the
approle_custom_mounttests, because now all tests use custom mounts (a unique mount point name per test)Kind
/kind cleanup
Release Note