Enhance existing eventlistener to support PodTemplate for Deployment using duck type #734

savitaashture · 2020-08-31T14:02:46Z

Changes

As described in the issue the feature is part of TEP and addresses supporting podtemplate duck type to kubernetes resources.

Signed-off-by: Savita Ashture sashture@redhat.com

/kind feature

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

A new field resources has been introduced as part of EventListener spec

apiVersion: triggers.tekton.dev/v1alpha1
kind: EventListener
metadata:
  name: github-listener-interceptor
spec:
  triggers:
    ...
  resources:
    kubernetesResource:
      serviceType: NodePort
      spec:
        template:
          metadata:
            labels:
              key: "value"
            annotations:
              key: "value"
          spec:
            serviceAccountName: tekton-triggers-github-sa
            nodeSelector:
              app: test
            tolerations:
              - key: key
                value: value
                operator: Equal
                effect: NoSchedule

resources field mainly contains information that whether the resource is built in kubernetes or custom

As of now resources field support kubernetesResource which helps to use PodSpecable ducktype and with the help of ducktype can avoid specifying serviceAccountName, serviceType and podTemplate as part of eventlistener spec

As of now for backward compatibility both way supported and resources are optional.

Once we deprecate old way then resources field becomes mandatory if anyone wants to specify serviceAccountName, serviceType and podSpec.

Advantages

Including kubernetesResource as part of resources helps to make use of PodSpecable duck type
and this way we can avoid hardcoding of podSpec fields as part of podTemplate.
Including customResource as part of resources will help triggers to make use of any CRD ex: Knative Service which helps triggers to get the serverless functionality.
this is not supported as part of this PR but will continue as part of this TEP

savitaashture · 2020-08-31T14:03:11Z

/cc @dibyom

tekton-robot · 2020-08-31T14:04:44Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	93.4%	95.4%	1.9
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	73.7%	75.4%	1.7
test/builder/eventlistener.go	78.8%	81.5%	2.8

tekton-robot · 2020-08-31T14:21:30Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	93.4%	95.4%	1.9
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	73.7%	75.4%	1.7
test/builder/eventlistener.go	78.8%	81.5%	2.8

tekton-robot · 2020-08-31T15:18:18Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	93.4%	95.4%	1.9
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	73.7%	75.4%	1.7
test/builder/eventlistener.go	78.8%	81.5%	2.8

zxDiscovery · 2020-09-01T07:49:35Z

@savitaashture: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2020-09-01T09:05:37Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	93.4%	95.4%	1.9
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	74.1%	75.8%	1.6
test/builder/eventlistener.go	78.8%	81.5%	2.8

dibyom · 2020-09-01T14:54:06Z

@savitaashture are you planning on getting this for v0.8 or v0.9?

zxDiscovery · 2020-09-08T07:49:34Z

@savitaashture: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2020-09-13T09:02:47Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	100.0%	97.8%	-2.2
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	76.2%	78.3%	2.1

tekton-robot · 2020-09-13T11:52:32Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/event_listener_validation.go	100.0%	97.8%	-2.2
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	76.2%	78.3%	2.1
test/builder/eventlistener.go	79.3%	81.9%	2.6

tekton-robot · 2020-09-13T18:12:32Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	76.2%	78.3%	2.1
test/builder/eventlistener.go	79.3%	81.9%	2.6

docs/eventlisteners.md

pkg/apis/triggers/v1alpha1/event_listener_validation.go

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

tekton-robot · 2020-09-14T17:56:23Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	76.2%	78.3%	2.1
test/builder/eventlistener.go	79.3%	81.9%	2.6

docs/eventlisteners.md

pkg/apis/triggers/v1alpha1/event_listener_validation.go

pkg/apis/triggers/v1alpha1/event_listener_validation_test.go

dibyom · 2020-09-15T19:16:22Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

+	// for backward compatibility with original behavior
+	var serviceType = el.Spec.ServiceType
+	if el.Spec.Resources.KubernetesResource != nil && el.Spec.Resources.KubernetesResource.ServiceType != "" {
+		serviceType = el.Spec.Resources.KubernetesResource.ServiceType


Do we also want to add some defaulting logic in el.SetDefaults to change el.Spec.ServiceType to el.Spec.Resources.KubernetesResource.ServiceType?

I think we don't require because even if ServiceType is empty k8s itself set ClusterIP as default

WDYT?

Oh, what I meant by defaulting is something like

if el.Spec.ServiceType != nil { el.Spec.Resources.KubernetesResource.ServiceType = el.Spec.ServiceType

This is so that we can deprecate/remove the el.Spec.ServiceType field later.

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

dibyom · 2020-09-15T19:33:00Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

+		serviceAccountName                   string
+	)
+	podlabels = mergeMaps(el.Labels, GenerateResourceLabels(el.Name))
+


same comment as with serviceType -> it seems like we could also do some of this in the webhook via EL's SetDefaults.

dibyom · 2020-09-15T19:37:14Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

+		if el.Spec.Resources.KubernetesResource.Template.Spec.ServiceAccountName != "" {
+			serviceAccountName = el.Spec.Resources.KubernetesResource.Template.Spec.ServiceAccountName
+		}
+		annotations = el.Spec.Resources.KubernetesResource.Template.Annotations


Don't we also have to mergeMaps for annotations too like we do in 7622d51

I feel we don't require here because these are part of deployment.spec.template.metadata and there won't be any annotations which are added by k8s, So we don't get continuous loop issue.

So deployment.spec.template.metadata.annotation will add/update to annotations value if provided as part of el.Spec.Resources.KubernetesResource.Template.Annotations

even if user manually edit those directly on deployment it will not allow same like others.

Let me know if you have some other point to address

Hmm, yeah you are right...I was confusing deployment.spec.template.metadata.annotations with deployment.metadata.annotations. I think this is fine then 👍

Kind of off topic, the reconciler code is getting kind of long and unwieldy, we might want to see how we can refactor/clean it up a bit. I'll open an issue.

…using duck type

tekton-robot · 2020-09-16T13:22:27Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	76.2%	77.6%	1.4
test/builder/eventlistener.go	79.3%	81.9%	2.6

dibyom · 2020-09-17T18:52:58Z

test/builder/eventlistener.go

@@ -313,3 +320,37 @@ func EventListenerCELOverlay(key, expression string) EventInterceptorOp {
 		}
 	}
 }
+
+// EventListenerResources set specified resources to the EventListener.


In pipelines, we've been trying to get rid of test builders and use structs as much as possible. For our reconciler tests too, we've started using single make(EL/Deployment/Service) functions. I think its ok to keep for now, but later we might end up removing builders just like in Pipeline.

dibyom · 2020-09-17T18:53:50Z

Just a couple of small unresolved issues around defaulting (I think its ok to add them in a follow up if you want!)
/approve

tekton-robot · 2020-09-17T18:53:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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:

~~OWNERS~~ [dibyom]

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

savitaashture · 2020-09-21T14:19:33Z

Just a couple of small unresolved issues around defaulting (I think its ok to add them in a follow up if you want!)

I shall add it in follow up PR

Thank you 👍

dibyom · 2020-09-21T19:08:59Z

/lgtm

tekton-robot requested review from dlorenc and khrm August 31, 2020 14:02

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 31, 2020

tekton-robot requested a review from dibyom August 31, 2020 14:03

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 31, 2020

savitaashture force-pushed the issue-727 branch from f16278a to 048bb6b Compare August 31, 2020 14:11

savitaashture force-pushed the issue-727 branch from 048bb6b to a4d9fff Compare August 31, 2020 15:16

savitaashture force-pushed the issue-727 branch from a4d9fff to c89b35f Compare September 1, 2020 09:03

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2020

savitaashture force-pushed the issue-727 branch from c89b35f to 7e23367 Compare September 13, 2020 09:00

savitaashture force-pushed the issue-727 branch from 7e23367 to ee8a9f9 Compare September 13, 2020 11:50

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2020

savitaashture changed the title ~~Enhance existing eventlistener to support PodTemplate for Deployment using duck type~~ [WIP] Enhance existing eventlistener to support PodTemplate for Deployment using duck type Sep 13, 2020

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 13, 2020

savitaashture force-pushed the issue-727 branch from ee8a9f9 to 5f3813f Compare September 13, 2020 18:09

savitaashture changed the title ~~[WIP] Enhance existing eventlistener to support PodTemplate for Deployment using duck type~~ Enhance existing eventlistener to support PodTemplate for Deployment using duck type Sep 13, 2020

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2020

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

tekton-robot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 13, 2020

dibyom reviewed Sep 14, 2020

View reviewed changes

docs/eventlisteners.md Show resolved Hide resolved

pkg/apis/triggers/v1alpha1/event_listener_validation.go Show resolved Hide resolved

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go Outdated Show resolved Hide resolved

savitaashture force-pushed the issue-727 branch from 5f3813f to 6fa5c4b Compare September 14, 2020 17:52

dibyom reviewed Sep 15, 2020

View reviewed changes

Enhance existing eventlistener to support PodTemplate for Deployment …

10fc888

…using duck type

savitaashture force-pushed the issue-727 branch from 6fa5c4b to 10fc888 Compare September 16, 2020 13:20

dibyom reviewed Sep 17, 2020

View reviewed changes

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

savitaashture mentioned this pull request Sep 21, 2020

EventListener should allow to set resource requirements #762

Closed

tekton-robot assigned dibyom Sep 21, 2020

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

tekton-robot merged commit 8d2ba7d into tektoncd:master Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance existing eventlistener to support PodTemplate for Deployment using duck type #734

Enhance existing eventlistener to support PodTemplate for Deployment using duck type #734

savitaashture commented Aug 31, 2020 •

edited

Loading

savitaashture commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

zxDiscovery commented Sep 1, 2020

tekton-robot commented Sep 1, 2020

dibyom commented Sep 1, 2020

zxDiscovery commented Sep 8, 2020

tekton-robot commented Sep 13, 2020

tekton-robot commented Sep 13, 2020

tekton-robot commented Sep 13, 2020

tekton-robot commented Sep 14, 2020

dibyom Sep 15, 2020

savitaashture Sep 16, 2020 •

edited

Loading

dibyom Sep 17, 2020

dibyom Sep 15, 2020

dibyom Sep 15, 2020

savitaashture Sep 16, 2020

dibyom Sep 17, 2020

tekton-robot commented Sep 16, 2020

dibyom Sep 17, 2020

dibyom commented Sep 17, 2020

tekton-robot commented Sep 17, 2020

savitaashture commented Sep 21, 2020

dibyom commented Sep 21, 2020

Enhance existing eventlistener to support PodTemplate for Deployment using duck type #734

Enhance existing eventlistener to support PodTemplate for Deployment using duck type #734

Conversation

savitaashture commented Aug 31, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

Advantages

savitaashture commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

zxDiscovery commented Sep 1, 2020

tekton-robot commented Sep 1, 2020

dibyom commented Sep 1, 2020

zxDiscovery commented Sep 8, 2020

tekton-robot commented Sep 13, 2020

tekton-robot commented Sep 13, 2020

tekton-robot commented Sep 13, 2020

tekton-robot commented Sep 14, 2020

dibyom Sep 15, 2020

Choose a reason for hiding this comment

savitaashture Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

dibyom Sep 17, 2020

Choose a reason for hiding this comment

dibyom Sep 15, 2020

Choose a reason for hiding this comment

dibyom Sep 15, 2020

Choose a reason for hiding this comment

savitaashture Sep 16, 2020

Choose a reason for hiding this comment

dibyom Sep 17, 2020

Choose a reason for hiding this comment

tekton-robot commented Sep 16, 2020

dibyom Sep 17, 2020

Choose a reason for hiding this comment

dibyom commented Sep 17, 2020

tekton-robot commented Sep 17, 2020

savitaashture commented Sep 21, 2020

dibyom commented Sep 21, 2020

savitaashture commented Aug 31, 2020 •

edited

Loading

savitaashture Sep 16, 2020 •

edited

Loading