Set default value to TektonConfig object #415
Conversation
tekton-robot
added
the
release-note-none
tekton-robot
added
the
size/XS
| @@ -108,6 +108,8 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, tc *v1alpha1.TektonConfi | |||
| return nil | |||
| } | |||
|
|
|||
| tc.SetDefaults(ctx) | |||
There was a problem hiding this comment.
@vdemeester when we pass the object through defaulting in reconciler, do we also update the object in etcd?
There was a problem hiding this comment.
i think the defaulter won't do it. however, the next update action down in the code/reconciler will get it into etcd.
so your concern is right.
The change in this patch should be moved to here
There was a problem hiding this comment.
in fact, we can do it in both places, it should have the same effect functionaly.
There was a problem hiding this comment.
im fine with merging this as it is.
There was a problem hiding this comment.
addressed the comment btw
There was a problem hiding this comment.
We have the same in tektoncd/pipeline, as @nikhil-thomas said, it won't be stored in etcd but ain't such a big deal.
vinamra28
force-pushed
the
issues-368
branch
from
5b64361
to
9a80ec5
Compare
nikhil-thomas
removed
the
release-note-none
|
/approve |
tekton-robot
added
the
approved
| @@ -120,6 +120,7 @@ func (tc tektonConfig) createInstance() error { | |||
| }, | |||
| }, | |||
| } | |||
| tcCR.SetDefaults(context.TODO()) | |||
There was a problem hiding this comment.
I think this should use ctx, meaning createInstance should take a context.Context as argument, otherwise we colud miss actual configured defaults
There was a problem hiding this comment.
also there is line where context.TODO is used. Shall I change that also?
There was a problem hiding this comment.
I don't see the line but.. maybe, yeah ?
There was a problem hiding this comment.
maybe it was moved with my commit. I have changed that with the ctx here
| @@ -108,6 +108,8 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, tc *v1alpha1.TektonConfi | |||
| return nil | |||
| } | |||
|
|
|||
| tc.SetDefaults(ctx) | |||
There was a problem hiding this comment.
We have the same in tektoncd/pipeline, as @nikhil-thomas said, it won't be stored in etcd but ain't such a big deal.
vinamra28
force-pushed
the
issues-368
branch
from
9a80ec5
to
f9c32e8
Compare
Currently when running the Tekton operator if the controller pod is up and webhook pod is not ready the controller will create the TektonConfig object and that TektonConfig object will not have any default values. This PR add the default values to TektonConfig object whenever the CR get's created by the Controller so that even if the webhook pod is not ready the TektonConfig CR can still have the default values.
vinamra28
force-pushed
the
issues-368
branch
from
f9c32e8
to
a6e1c45
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nikhil-thomas, vdemeester 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 |
Changes
Currently when running the Tekton operator if the controller pod is up
and webhook pod is not ready the controller will create the TektonConfig
object and that TektonConfig object will not have any default values.
This PR add the default values to TektonConfig object whenever the CR
get's created by the Controller so that even if the webhook pod is not
ready the TektonConfig CR can still have the default values.
Closes #368
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes