Use Listers to fetch data in the Sink #821
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tekton-robot
added
the
release-note
tekton-robot
added
size/L
|
This is great! Thank you! For the build tests, it looks like the issue is: So, we should handle the error value instead of ignoring it. For the integration test failure, looks like the issue has something to do with the serviceAccountName permissions: The e2e test creates its own ServiceAccount and Roles and uses that in the EventListener that the test creates. Currently, that role only has access to "get" triggers, bindings etc. With this change, it also needs access to "list" these resources: https://github.com/tektoncd/triggers/blob/master/test/eventlistener_test.go#L244 |
dibyom
reviewed
Nov 2, 2020
dibyom
reviewed
Nov 2, 2020
tekton-robot
added
needs-rebase
yaoxiaoqi
force-pushed
the
use-listers
branch
2 times, most recently
from
2d6f74b to
9c80de4
Compare
|
/test pull-tekton-triggers-integration-tests |
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
yaoxiaoqi
force-pushed
the
use-listers
branch
from
9c80de4 to
f938963
Compare
dibyom
reviewed
Nov 5, 2020
examples/role-resources/clustertriggerbinding-roles/clusterrole.yaml
Outdated
Show resolved
Hide resolved
| @@ -138,4 +138,35 @@ func createServiceAccount(t *testing.T, c *clients, namespace, name string) { | |||
| t.Fatalf("Error creating RoleBinding: %s", err) | |||
| } | |||
|
|
|||
| _, err = c.KubeClient.RbacV1().ClusterRoles().Create(context.Background(), | |||
| &rbacv1.ClusterRole{ | |||
There was a problem hiding this comment.
Since we are creating cluster-scoped resources, we should also delete them in a cleanup function. (By default we delete the namespace which deletes all the namespaced resources but not the cluster ones).
(e.g. we have a cleanup https://github.com/tektoncd/triggers/blob/master/test/eventlistener_test.go#L608 that is then called in the test: https://github.com/tektoncd/triggers/blob/master/test/eventlistener_test.go#L127)
There was a problem hiding this comment.
Got it. Besides, I'm curious about https://github.com/tektoncd/triggers/blob/master/test/eventlistener_test.go#L608. I would like to know why we need to delete eventlistener after the whole namespace is already deleted. It seems that the eventlistener lives along with the namespace.
There was a problem hiding this comment.
hmm, that's a good question 🤔
We probably should just delete the EL first and then the namespace? Or just the namespace.
There was a problem hiding this comment.
So, if TEST_KEEP_NAMESPACES env var is set, we will not delete the namespace but will delete the EL and the servcie . I wonder if this is intentional behavior.
There was a problem hiding this comment.
Thanks for the explanation. It makes sense. Should I also delete the eventlistener in test/eventlistener_scale_test.go after tear down the namespace? I can open another PR.
There was a problem hiding this comment.
Actually, maybe we can just remove the lines that delete the EventListener and instead rely on the cleanup that already deletes the namespace to clean it up (in TestEventListenerCreate)?
This commit closes tektoncd#797. This PR uses Listers to get resources in the Eventlistener Sink, rather than directly making calls to the API server. Currently, multiple informers need to be setup when creating a Sink object. But the Sink in the cmd/triggerrun still makes the direct API calls because it only makes the call once when executed, which means Lister cache is not useful in this case. All examples work under this change.
yaoxiaoqi
force-pushed
the
use-listers
branch
from
f938963 to
f7dcb09
Compare
|
/approve |
|
[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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
This PR uses Listers to fetch data from EventListeners, Triggers, TriggerBindings, TriggerTemplates, and
ClusterTriggerBindings, rather than directly making calls to the API server. Currently, multiple informers need to be
specified when creating a Sink object.
All Triggers in examples/ work under this change.
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