✨ Make clusterctl cert-manager timeout configurable #2834
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/cc @fabriziopandini |
There was a problem hiding this comment.
@nader-ziada
Considering that cert-manager-timeout is a simple variable, IMO we can do a much simpler solution using o.configClient.Variables instead of introducting a full fladged CertManagerTimeoutClient.
On the contrary, the CertManagerTimeoutClient can be justified only if we plan to support many other timeouts in the future, but in this case we should create a generic TimeoutClient backed by a configuration entry with type map[string]time.Duration
@nader-ziada @vincepri @wfernandes opinions?
| func (cm *certManagerClient) getWaitTimeout() time.Duration { | ||
| log := logf.Log | ||
|
|
||
| timeout, err := cm.configClient.CertManagerTimeout().Get() |
There was a problem hiding this comment.
Can we do o.configClient.Variables.Get(overrideFolderKey) like we are already doing for e.g. overrideFolders
There was a problem hiding this comment.
I agree with leveraging the Variables Client.
As an example if you define certManagerTimeout similar to the overridesFolder in clusterctl.yaml. It should be made accessible via the variables client. Then we don't need ot have a specific client just for a component's timeout.
There was a problem hiding this comment.
ok, I was just trying to follow the pattern I found, will re-work it
nader-ziada
force-pushed
the
cert-manager-timeout
branch
from
fd39aad to
dec981a
Compare
|
@fabriziopandini @wfernandes made the switch to variables, please take another look |
|
|
||
| timeout, err := cm.configClient.Variables().Get(timeoutConfigKey) | ||
| if err != nil { | ||
| return waitCertManagerTimeout |
There was a problem hiding this comment.
nit (not blocking) waitCertManagerDefaultTimeout
- cert-manager-timeout can be set in config file - default of 10 minutes if no value is set
nader-ziada
force-pushed
the
cert-manager-timeout
branch
from
dec981a to
cd655cd
Compare
k8s-ci-robot
added
the
lgtm
|
/approve per @fabriziopandini approval above |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini, nader-ziada, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
**Which issue(s) this PR fixes
Fixes #2822