Refactor TektonConfig Reconciler and Cleanup #421

sm43 · 2021-09-24T04:13:44Z

Fixes addon component deletion

If clustertask and pipelines templates are disabled then
due to return statement it used to return early without
executing remaning code.

Updates TektonConfig Conditions and cleanup

This updates the tektonconfig conditions and removes
unnecessary code

Removes deletion timestamp check in Autocreation of TektonConfig

If TektonConfig has a deletion stamp, it won't be deleted
unless the operator is up and running as the object has
finalizer. waiting for it to get deleted will stop operator
from running and object still won't be deleted.
hence removing the condition for checking deletion timestamp
and letting user to create a new instance of tektonconfig
after deletion.

Signed-off-by: Shivam Mukhade smukhade@redhat.com

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

If clustertask and pipelines templates are disabled then due to return statement it used to return early without executing remaning code. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

tekton-robot · 2021-09-24T04:16:49Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonaddon_types.go	0.0%	33.3%	33.3
pkg/apis/operator/v1alpha1/tektonconfig_lifecycle.go	76.5%	40.0%	-36.5
pkg/apis/operator/v1alpha1/tektonconfig_types.go	50.0%	33.3%	-16.7

savitaashture · 2021-09-24T05:28:56Z

pkg/apis/operator/v1alpha1/tektonconfig_lifecycle.go

+		PreReconciler,
+		ComponentsReady,
+		PostReconciler,


I assume these conditions are displayed on status and i think PreReconciler and PostReconciler words are more towards technical

May i know the reason behind removing old conditions

PreReconciler and PostReconciler words are more towards technical

I agree with this. It would be great if we could rename it with something that indicates the objective/end result of the action. It doesn't has to be precise. Even PreInstall PostInstall will be better.

yeah PreInstall PostInstall will also be nice
also we do want to break down the component status in some way
currently the componentsready cond will show any error
but in further interation we do want to find a better way for it

Note that if we change what is written in the Status (the name of the condition, …), we need to validate other tools that are depending on this won't be broken (e.g. openshift console, …)

(because we are changing the API)

yeah in this case we are chaning the api
we have removed the manifest field from status
and updated conditions
what tools do we have to validate? 🤔

pkg/apis/operator/v1alpha1/tektonconfig_lifecycle.go

pkg/apis/operator/v1alpha1/tektonconfig_types.go

pkg/reconciler/openshift/tektonaddon/tektonaddon.go

pkg/reconciler/kubernetes/tektonconfig/extension.go

pkg/reconciler/openshift/tektonconfig/extension.go

nikhil-thomas · 2021-09-24T07:26:45Z

/approve

sm43 · 2021-09-24T07:28:02Z

/hold
to rename the conditions

vdemeester · 2021-09-24T08:29:58Z

pkg/apis/operator/v1alpha1/tektonaddon_types.go

+}
+
+func (a Addon) IsEmpty() bool {
+	return reflect.DeepEqual(a, Addon{})


Any reason to use reflect here instead of just len(a.Params) == 0 ?
We should avoid using reflect when we can, it's kind-of costly for.. not always a valid reason to be.

yeah len of param would work...
@vdemeester how about using equality.Semantic.DeepEqual not in this case but here is that okay compared to reflect?

Maybe yes ? My point is just that we should aim to use as less as possible reflect and those things.. but when we don't have the choice .. 😛

vdemeester · 2021-09-24T08:31:43Z

pkg/apis/operator/v1alpha1/tektonconfig_lifecycle.go

+		PreReconciler,
+		ComponentsReady,
+		PostReconciler,


Note that if we change what is written in the Status (the name of the condition, …), we need to validate other tools that are depending on this won't be broken (e.g. openshift console, …)

vdemeester · 2021-09-24T08:31:53Z

pkg/apis/operator/v1alpha1/tektonconfig_lifecycle.go

+		PreReconciler,
+		ComponentsReady,
+		PostReconciler,


(because we are changing the API)

pkg/apis/operator/v1alpha1/tektonconfig_lifecycle_test.go

vdemeester · 2021-09-24T08:33:59Z

pkg/apis/operator/v1alpha1/tektonconfig_lifecycle.go

-// GroupVersionKind returns SchemeGroupVersion of a TektonConfig
-func (tp *TektonConfig) GroupVersionKind() schema.GroupVersionKind {
+func (tc *TektonConfig) GroupVersionKind() schema.GroupVersionKind {
 	return SchemeGroupVersion.WithKind(KindTektonConfig)
 }

-// GetCondition returns the current condition of a given condition type
-func (tps *TektonConfigStatus) GetCondition(t apis.ConditionType) *apis.Condition {
-	return configCondSet.Manage(tps).GetCondition(t)
+func (tc *TektonConfig) GetGroupVersionKind() schema.GroupVersionKind {
+	return SchemeGroupVersion.WithKind(KindTektonConfig)


Any reason to have both ?

GroupVersionKind can be removed now.. will update !

in generated code and internally in knative/pkg GroupVersionKind is used, in some transformer
GetGroupVersionKind is defined so that we can used in FilterController

tektonTriggerinformer.Get(ctx).Informer().AddEventHandler(cache.FilteringResourceEventHandler{ FilterFunc: controller.FilterController(&v1alpha1.TektonConfig{}), Handler: controller.HandleAll(impl.EnqueueControllerOf), })

so lets keep GroupVersionKind also
i will take a deep look and then remove

pkg/apis/operator/v1alpha1/tektonconfig_types.go

tekton-robot · 2021-09-24T09:25:13Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonaddon_types.go	0.0%	60.0%	60.0
pkg/apis/operator/v1alpha1/tektonconfig_lifecycle.go	76.5%	40.0%	-36.5
pkg/apis/operator/v1alpha1/tektonconfig_types.go	50.0%	33.3%	-16.7

This updates the tektonconfig conditions and removes unnecessary code. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

If TektonConfig has a deletion stamp, it won't be deleted unless the operator is up and running as the object has finalizer. waiting for it to get deleted will stop operator from running and object still won't be deleted. hence removing the condition for checking deletion timestamp and letting user to create a new instance of tektonconfig after deletion. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

tekton-robot · 2021-09-24T09:55:54Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/operator/v1alpha1/tektonaddon_types.go	0.0%	33.3%	33.3
pkg/apis/operator/v1alpha1/tektonconfig_lifecycle.go	76.5%	40.0%	-36.5
pkg/apis/operator/v1alpha1/tektonconfig_types.go	50.0%	33.3%	-16.7

sm43 · 2021-09-24T10:35:42Z

/hold cancel

vdemeester · 2021-09-24T10:47:20Z

/approve

tekton-robot · 2021-09-24T10:47:26Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [nikhil-thomas,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nikhil-thomas · 2021-09-24T11:42:18Z

/lgtm

Fixes addon component deletion

756f724

If clustertask and pipelines templates are disabled then due to return statement it used to return early without executing remaning code. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

tekton-robot requested review from piyush-garg and pradeepitm12 September 24, 2021 04:13

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 24, 2021

savitaashture reviewed Sep 24, 2021

View reviewed changes