Support apply and delete of cert-manager #320
Conversation
gthao313
force-pushed
the
integ-test-fix
branch
2 times, most recently
from
3c9a6dd
to
adb475e
Compare
integ/src/main.rs
Outdated
| process_cert_manager(Action::Apply, &kube_config_path) | ||
| .await | ||
| .context(error::RunBrupopSnafu)?; | ||
| sleep(WAIT_CERT_MANAGER_COMPLETE); |
There was a problem hiding this comment.
| sleep(WAIT_CERT_MANAGER_COMPLETE); | |
| sleep(Duration::from_secs(90)); |
Since this only gets called once and is specific to waiting for cert-manager to complete, could we just have the duration set here instead of declaring a constant?
There was a problem hiding this comment.
I think we should avoid using magical number, name the variable can be more explanatory。
There was a problem hiding this comment.
Probably doesn't matter but you never know when blocking an async thread could cause a problem. std::thread::sleep is blocking, but toklo::time::sleep is async.
There was a problem hiding this comment.
but toklo::time::sleep is async.
Interesting!
integ/src/main.rs
Outdated
| process_cert_manager(Action::Apply, &kube_config_path) | ||
| .await | ||
| .context(error::RunBrupopSnafu)?; | ||
| sleep(WAIT_CERT_MANAGER_COMPLETE); |
There was a problem hiding this comment.
Probably doesn't matter but you never know when blocking an async thread could cause a problem. std::thread::sleep is blocking, but toklo::time::sleep is async.
| let action_string: String = action.to_string(); | ||
|
|
||
| // install cert-manager | ||
| let cert_manager_status = Command::new(KUBECTL_BINARY) |
There was a problem hiding this comment.
It seems weird to me to do things with kubectl instead of kube-rs, but I guess that's already the pattern.
There was a problem hiding this comment.
It seems weird to me to do things with kubectl
This seems like a pretty reasonable punt: the alternative with kube-rs would be to
- Deserialize the cert-manager yaml into rust objects that can be consumed by
kube-rs - Create our own client via
kube-rs - Apply those objects to the cluster via our
kube-rsclient - Wait / validate objects via
kube-rsclient
(essentially all the things kubectl does for us with just a static yaml manifest)
But I guess these integration tests sort of sit somewhere between a "script" and an actual program you'd compile/run. The script part of it assumes there's alot of stuff on your local host to actually accomplish the integration tests (access to the brupop testing accounts, kubectl, etc.)
Maybe this will be due for a refactor in the future but for now, I agree, I think this is ok.
There was a problem hiding this comment.
I think that's a reasonable ask. I remember we have that kind conversation before and I agreed to use kube-rs in future. I've created a new issue to tack this. Thanks.
gthao313
force-pushed
the
integ-test-fix
branch
from
adb475e
to
afe946f
Compare
|
Push above using |
Issue number:
Closes: #319
Description of changes:
Support apply and delete of cert-manager
Testing done:
Integration test
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.