Propagate annotations from EL to svc and deployment #712

piyush-garg · 2020-08-14T22:13:20Z

This will add the feature of propagating annotations
of an eventlistener to the service, deployment and
further pods created by the eventlistener

Add docs and tests

Fix #700

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

Now annotations of the eventlistener will be propagated to deployment and services. If you have any custom annotation on your services/deployment, please add them to annotations otherwise they will be overwritten.

tekton-robot · 2020-08-14T22:14:50Z

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	70.8%	71.3%	0.5

piyush-garg · 2020-08-14T22:15:14Z

/cc @dibyom @khrm @savitaashture

savitaashture · 2020-08-17T04:23:13Z

@piyush-garg Thank you for the PR 👍

I have one suggestion related to annotation usage as @dibyom mentioned use case of adding annotation over the issue so i feel it would be great if we cover that usecase as part of examples also

Otherwise PR looks good to me 🙂

khrm

lgtm but add this information succinctly in https://github.com/tektoncd/triggers/blob/master/docs/eventlisteners.md
And an example like @savitaashture said.

piyush-garg · 2020-08-17T05:25:47Z

@khrm @savitaashture Would want to add some annotation to all the examples there? The feature is all the annotations present in eventlistener will be copied to all services and deployments, not sure how we can show that in examples.

savitaashture · 2020-08-17T06:35:53Z

@khrm @savitaashture Would want to add some annotation to all the examples there? The feature is all the annotations present in eventlistener will be copied to all services and deployments, not sure how we can show that in examples.

IMO adding in one example(GH, GL etc...) would be enough to show the usage of annotation propagation.
I mean you can take the examples from here and show how we can use that as part of EL in README is also fine because adding it in examples will be difficult because of dependency.

And +1 for adding it to docs

piyush-garg · 2020-08-17T19:00:17Z

@khrm @savitaashture I have updated the PR with doc and example. Thanks for review

tekton-robot · 2020-08-17T19:02:04Z

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	70.8%	71.3%	0.5

khrm · 2020-08-18T17:36:27Z

/lgtm

savitaashture · 2020-08-18T17:39:59Z

/lgtm

Thank you

dibyom · 2020-08-19T15:09:01Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

@@ -447,6 +451,7 @@ func generateObjectMeta(el *v1alpha1.EventListener) metav1.ObjectMeta {
 		Name:            el.Status.Configuration.GeneratedResourceName,
 		OwnerReferences: []metav1.OwnerReference{*el.GetOwnerReference()},
 		Labels:          mergeLabels(el.Labels, GenerateResourceLabels(el.Name)),
+		Annotations:     el.Annotations,


I think we should merge any extra annotations that might have been added by something besides the EL controller instead of overwriting it (just like we are doing it with the labels above).

@dibyom We are also overwriting in case of labels as per my understanding. We are merging labels to add the few labels which are generated by triggers controller, apart from that in case of labels also we are doing the same, copying the available in eventlistener, add the few generated by the controller and propagate to svc/deployment.

If we merge the extra annotation, don't do any anything which is already added, then how we can remove the one which a user did by mistake e.g. I have added an annotation with a typo in key. We will be in the same situation as before. So I think whatever the current set in EL should be copied further.

And as per my little understanding with the codebase, we are doing the same with labels. Please correct me here if wrong.

Yes we are just copying labels coming from eventlistener and merging with labels which are generated by triggers controller.
For annotation we don't have any annotation which are generated by triggers controller so we should just copy whatever is coming from eventlistener yaml and imo we don't require any merging functionality here

I think we should merge any extra annotations that might have been added by something besides the EL controller instead of overwriting it

@dibyom when you say something besides the EL controller do you mean annotation added by user ?

Hey, sorry for the delayed response.

@piyush-garg you are right -> we do overwrite the labels. The use case I was thinking of is the following -- say someone currently has a custom annotation set directly on the EL service/deployment. Now, when they upgrade to Triggers containing this change, the custom annotation will be unset. So, they'd have to manually add their annotations to the EL in order to make it work.

If we merge the extra annotation, don't do any anything which is already added, then how we can remove the one which a user did by mistake e.g. I have added an annotation with a typo in key.

Yeah, that's not ideal either :( The user would have to delete them from the EL deployment/service manually.

It gets a bit more complicated if we consider TEP-009. In that design, users can specify annotations/labels directly for the EL deployment. So, we'll have to see if it makes sense to keep propagating them.

So, the options I see are:

Make the change in this PR and mark in the Release Notes that users have to manually update the EL's annotations if they added any custom annotations.

Merge any existing annotations on the labels/annotations with any applied on the EL instead of overwriting them.

Wait for TEP-009 design to be implemented where users can directly specify a podTemplate with exactly the annotations they want for the deployment. ( We might want to augment that design with something like a serviceTemplate for the EL service as well)

Does that make sense? Thoughts @savitaashture @piyush-garg @khrm ?

@dibyom

Yes i agree with your point lets merge this PR with release note

Wait for TEP-009 design to be implemented where users can directly specify a podTemplate with exactly the annotations they want for the deployment. ( We might want to augment that design with something like a serviceTemplate for the EL service as well)

Yes need to rethink about the support of annotation for Service while implementing TEP-0008 as that is bit tricky.
I will take care of this while implementation
Thank you for pointing it out here 👍

savitaashture · 2020-08-21T14:26:38Z

@piyush-garg could you please add points mentioned by @dibyom to release note and also resolve the conflict

tekton-robot · 2020-08-27T18:28:51Z

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	73.7%	74.1%	0.5

piyush-garg · 2020-08-27T18:31:39Z

@khrm @savitaashture @dibyom I have updated the PR according to the discussion and also resolved the conflicts. Thanks for all reviews and suggestions.

savitaashture · 2020-08-28T03:16:12Z

/lgtm

khrm · 2020-08-28T13:45:56Z

docs/eventlisteners.md

@@ -220,6 +221,24 @@ must adhere to the
 [Kubernetes syntax and character set requirements](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set)
 for label values.

+## Annotations


I think we should also document the behaviour for what happens when deployment or service has an annotation already present.

Or is mentioning that only in release note OK, @dibyom ?

@khrm I have updated the doc also with the Note

This will add the feature of propagating annotations of an eventlistener to the service, deployment and further pods created by the eventlistener Add docs and tests Fix tektoncd#700

tekton-robot · 2020-08-31T07:37:03Z

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	73.7%	74.1%	0.5

khrm

/lgtm

piyush-garg · 2020-08-31T19:54:39Z

@savitaashture @dibyom Probably the last round of review

dibyom · 2020-08-31T22:18:58Z

/approve

tekton-robot · 2020-08-31T22:19:03Z

[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

In tektoncd#712, we added a feature to propagate annotations added to the EL to its underlying resources. However, this resulted in infinite reconcile loops since the deployment controller will add a standard revision annotation that the EL controller will keep trying to overwrite. To fix this, this commit switches the annotation propagation to merge any annotations set on the underlying resources before adding any extra annotations from the EL. Fixes tektoncd#751 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

In tektoncd#712, we added a feature to propagate annotations added to the EL to its underlying resources. However, this resulted in infinite reconcile loops since the deployment controller will add a standard revision annotation that the EL controller will keep trying to overwrite. To fix this, this commit switches the annotation propagation to merge any annotations set on the underlying resources before adding any extra annotations from the EL. Fixes tektoncd#752 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

In #712, we added a feature to propagate annotations added to the EL to its underlying resources. However, this resulted in infinite reconcile loops since the deployment controller will add a standard revision annotation that the EL controller will keep trying to overwrite. To fix this, this commit switches the annotation propagation to merge any annotations set on the underlying resources before adding any extra annotations from the EL. Fixes #752 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

tekton-robot requested review from ncskier and vtereso August 14, 2020 22:13

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 14, 2020

tekton-robot requested review from dibyom, khrm and savitaashture August 14, 2020 22:15

khrm reviewed Aug 17, 2020

View reviewed changes

piyush-garg force-pushed the fix_700 branch from ade8937 to 552f6f7 Compare August 17, 2020 18:59

tekton-robot assigned khrm Aug 18, 2020

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

tekton-robot assigned savitaashture Aug 18, 2020

dibyom reviewed Aug 19, 2020

View reviewed changes

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

piyush-garg force-pushed the fix_700 branch from 552f6f7 to c0ba824 Compare August 27, 2020 18:26

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2020

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

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

khrm reviewed Aug 28, 2020

View reviewed changes

Propagate annotations from EL to svc and deployment

909f589

This will add the feature of propagating annotations of an eventlistener to the service, deployment and further pods created by the eventlistener Add docs and tests Fix tektoncd#700

piyush-garg force-pushed the fix_700 branch from c0ba824 to 909f589 Compare August 31, 2020 07:35

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2020

khrm reviewed Aug 31, 2020

View reviewed changes

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

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

tekton-robot merged commit 0e93685 into tektoncd:master Aug 31, 2020

dibyom mentioned this pull request Sep 10, 2020

EL Reconciler keeps updating Deployment #752

Closed

dibyom mentioned this pull request Sep 10, 2020

Merge annotations before propagation #753

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate annotations from EL to svc and deployment #712

Propagate annotations from EL to svc and deployment #712

piyush-garg commented Aug 14, 2020 •

edited

Loading

tekton-robot commented Aug 14, 2020

piyush-garg commented Aug 14, 2020

savitaashture commented Aug 17, 2020

khrm left a comment

piyush-garg commented Aug 17, 2020

savitaashture commented Aug 17, 2020 •

edited

Loading

piyush-garg commented Aug 17, 2020

tekton-robot commented Aug 17, 2020

khrm commented Aug 18, 2020

savitaashture commented Aug 18, 2020

dibyom Aug 19, 2020

piyush-garg Aug 19, 2020

savitaashture Aug 20, 2020

dibyom Aug 21, 2020

savitaashture Aug 21, 2020 •

edited

Loading

savitaashture commented Aug 21, 2020 •

edited

Loading

tekton-robot commented Aug 27, 2020

piyush-garg commented Aug 27, 2020 •

edited

Loading

savitaashture commented Aug 28, 2020

khrm Aug 28, 2020

khrm Aug 28, 2020

piyush-garg Aug 31, 2020

tekton-robot commented Aug 31, 2020

khrm left a comment

piyush-garg commented Aug 31, 2020

dibyom commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

Propagate annotations from EL to svc and deployment #712

Propagate annotations from EL to svc and deployment #712

Conversation

piyush-garg commented Aug 14, 2020 • edited Loading

Submitter Checklist

Release Notes

tekton-robot commented Aug 14, 2020

piyush-garg commented Aug 14, 2020

savitaashture commented Aug 17, 2020

khrm left a comment

Choose a reason for hiding this comment

piyush-garg commented Aug 17, 2020

savitaashture commented Aug 17, 2020 • edited Loading

piyush-garg commented Aug 17, 2020

tekton-robot commented Aug 17, 2020

khrm commented Aug 18, 2020

savitaashture commented Aug 18, 2020

dibyom Aug 19, 2020

Choose a reason for hiding this comment

piyush-garg Aug 19, 2020

Choose a reason for hiding this comment

savitaashture Aug 20, 2020

Choose a reason for hiding this comment

dibyom Aug 21, 2020

Choose a reason for hiding this comment

savitaashture Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

savitaashture commented Aug 21, 2020 • edited Loading

tekton-robot commented Aug 27, 2020

piyush-garg commented Aug 27, 2020 • edited Loading

savitaashture commented Aug 28, 2020

khrm Aug 28, 2020

Choose a reason for hiding this comment

khrm Aug 28, 2020

Choose a reason for hiding this comment

piyush-garg Aug 31, 2020

Choose a reason for hiding this comment

tekton-robot commented Aug 31, 2020

khrm left a comment

Choose a reason for hiding this comment

piyush-garg commented Aug 31, 2020

dibyom commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

piyush-garg commented Aug 14, 2020 •

edited

Loading

savitaashture commented Aug 17, 2020 •

edited

Loading

savitaashture Aug 21, 2020 •

edited

Loading

savitaashture commented Aug 21, 2020 •

edited

Loading

piyush-garg commented Aug 27, 2020 •

edited

Loading