Fixes deployment on OpenShift #404

sm43 · 2021-09-09T07:01:50Z

Moves TektonConfig ExtendedController to a separate pkg

TektonConfig's ExtendedController is used in both platforms k8s and
OpenShift and was in the k8s pkg. K8s controller use Dashboard
informer but OpenShift's doesn't need it. As we import the Dashboard
Informer pkg in k8s it has an init func which get executed and looks
for Dashboard CRD. This was happening with OpenShift where we don't
install Dashboard CRD now. so to avoid dashboard informer getting
initialize for OpenShift moving ExtendedController to a separate pkg.

Enable TektonAddon Defaulting & Validation in Webhook

This fixes the failure of TektonAddon on OpenShift because of the
params not getting added to cr. this enables the defaulting
and validation in TektonConfig.

This also adds for k8s platform but it won't have any effect of the
installation of addon on k8s as it is not controller in not added
for k8s. This in plan to enable in upcoming days.

[OpenShift] Fixes Operator webhook

Earlier for Openshift, the specific files were not getting merged in
base due to which webhook was looking at tekton-operator ns instead
of openshift-operators, this fixes it.

And also updates the service account and rbac for webhook.

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

tekton-robot · 2021-09-09T07:05:15Z

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/reconciler/common-reconciler/tektonconfig/pipeline/pipeline.go	Do not exist	56.5%
pkg/reconciler/common-reconciler/tektonconfig/trigger/trigger.go	Do not exist	72.1%

tekton-robot · 2021-09-09T07:24:11Z

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/reconciler/common-reconciler/tektonconfig/pipeline/pipeline.go	Do not exist	56.5%
pkg/reconciler/common-reconciler/tektonconfig/trigger/trigger.go	Do not exist	72.1%

vdemeester

Not a fan of the package 😅 pkg/reconciler/common-reconciler/… there is common and a repetition there. What pkg/reconciler/shared/… instead ? (it's the "shared" code for all platform more or less).

Not to take in this PR, but NewExtendedController might be better named NewExtensibleController, shouldn't it ? 🤔

vdemeester · 2021-09-09T07:20:16Z

config/openshift/webhook.yaml

        app: tekton-operator
    spec:
-      serviceAccountName: tekton-operator
+      serviceAccountName: openshift-pipelines-operator


Same remark as I did to @nikhil-thomas's PR, I think this should stay tekton-operator as this is in tektoncd, OpenShift Pipelines happens to use this but we shouldn't make it hardcoded I think

ah okay. I think that change of service account was not reverted in that pr due to which webhook is failing now.
controller uses openshift-pipelines-operator and webhook tekton-operator
and on openshift tekton-operator is not created anymore

ah I see. Then we should probably create an issue to track this 😝

sm43 · 2021-09-09T07:34:28Z

Not a fan of the package sweat_smile pkg/reconciler/common-reconciler/… there is common and a repetition there. What pkg/reconciler/shared/… instead ? (it's the "shared" code for all platform more or less).

@vdemeester
yep shared is better. was not able to think of any 😅 will update. Thnx

Not to take in this PR, but NewExtendedController might be better named NewExtensibleController, shouldn't it ? thinking

yeah NewExtensibleController make sense

savitaashture · 2021-09-09T08:29:49Z

config/openshift/kustomization.yaml

+  - target:
+      kind: Deployment
+      name: tekton-operator
+    patch: |-
+      - op: replace
+        path: /metadata/name
+        value: openshift-pipelines-operator
+  - path: operator.yaml
+    target:
+      kind: Deployment
+      name: tekton-operator
+  - path: webhook.yaml
+    target:
+      kind: Deployment
+      name: tekton-operator-webhook
+  - path: role.yaml
+    target:
+      kind: ClusterRole
+      name: tekton-operator
+  - path: role_binding.yaml
+    target:
+      kind: ClusterRoleBinding
+      name: tekton-operator
+  - path: webhook.yaml
+    target:
+      kind: Deployment
+      name: tekton-operator-webhook


do we require this change ?

similarly in other places as well

ide identation 😅

pkg/reconciler/common-reconciler/tektonconfig/controller.go

TektonConfig's ExtendedController is used in both platforms k8s and OpenShift and was in the k8s pkg. K8s controller use Dashboard informer but OpenShift's doesn't need it. As we import the Dashboard Informer pkg in k8s it has an init func which get executed and looks for Dashboard CRD. This was happening with OpenShift where we don't install Dashboard CRD now. so to avoid dashboard informer getting initialize for OpenShift moving ExtendedController to a separate pkg. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

This fixes the failure of TektonAddon on OpenShift because of the params not getting added to cr. this enables the defaulting and validation in TektonConfig. This also adds for k8s platform but it won't have any effect of the installation of addon on k8s as it is not controller in not added for k8s. This in plan to enable in upcoming days. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

Earlier for Openshift, the specific files were not getting merged in base due to which webhook was looking at tekton-operator ns instead of openshift-operators, this fixes it. And also updtes the service account and rbac for webhook. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

tekton-robot · 2021-09-09T09:55: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/reconciler/shared/tektonconfig/pipeline/pipeline.go	Do not exist	56.5%
pkg/reconciler/shared/tektonconfig/trigger/trigger.go	Do not exist	72.1%

tekton-robot · 2021-09-09T09:58:53Z

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/reconciler/shared/tektonconfig/pipeline/pipeline.go	Do not exist	56.5%
pkg/reconciler/shared/tektonconfig/trigger/trigger.go	Do not exist	72.1%

pradeepitm12 · 2021-09-09T10:43:58Z

most of the review comments seem already addressed
/lgtm

tekton-robot · 2021-09-09T10:53:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [vdemeester]

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

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 9, 2021

tekton-robot requested review from piyush-garg and pradeepitm12 September 9, 2021 07:01

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 9, 2021

sm43 force-pushed the fix-os-deployment branch from 32931f2 to 2e9e27d Compare September 9, 2021 07:20

vdemeester requested changes Sep 9, 2021

View reviewed changes

savitaashture reviewed Sep 9, 2021

View reviewed changes

pkg/reconciler/common-reconciler/tektonconfig/controller.go Outdated Show resolved Hide resolved

Shivam Mukhade added 2 commits September 9, 2021 15:21

sm43 force-pushed the fix-os-deployment branch from 2e9e27d to 09f9ede Compare September 9, 2021 09:52

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 9, 2021

sm43 force-pushed the fix-os-deployment branch from 09f9ede to 78eceeb Compare September 9, 2021 09:55

tekton-robot assigned pradeepitm12 Sep 9, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021

sm43 requested review from savitaashture and vdemeester September 9, 2021 10:48

vdemeester approved these changes Sep 9, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2021

tekton-robot merged commit fcfa021 into tektoncd:main Sep 9, 2021

sm43 deleted the fix-os-deployment branch October 10, 2021 15:06

Fixes deployment on OpenShift #404

Fixes deployment on OpenShift #404

Uh oh!

Conversation

sm43 commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Sep 9, 2021

Uh oh!

tekton-robot commented Sep 9, 2021

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

sm43 Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

sm43 commented Sep 9, 2021

Uh oh!

savitaashture Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm43 Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tekton-robot commented Sep 9, 2021

Uh oh!

tekton-robot commented Sep 9, 2021

Uh oh!

pradeepitm12 commented Sep 9, 2021

Uh oh!

tekton-robot commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sm43 commented Sep 9, 2021 •

edited

Loading

sm43 Sep 9, 2021 •

edited

Loading

savitaashture Sep 9, 2021 •

edited

Loading