Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing the Triggers CRD TEP Status to Implementable #184

Merged
merged 1 commit into from
Sep 9, 2020

Conversation

khrm
Copy link
Contributor

@khrm khrm commented Aug 25, 2020

No description provided.

@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 25, 2020
@bobcatfish
Copy link
Contributor

bobcatfish commented Aug 26, 2020

I'm really excited about this feature in general but I do want to ask a few questions:

Selectors

  namespaceSelector:
     matchName: [ns-1, ns-2]
     matchLabels:
       - tekton-triggers: tekton-operator
  triggerSelector:
    matchLabels:
      - eventlistener: tekton-operator

I don't quite understand the difference between these two selectors and why they both involve labels. (Only the Triggers have labels on them, right?)

Are these based on a k8s precedent, and/or are there any alternative syntaxes that it's worth exploring?

Path format

To refer to trigger resource foo in namespace bar, we would have a url /bar?name=foo.

I think it might be worth talking about this path syntax a bit, are there any other options that have been discussed?

For example looking at the general syntax for query strings i'm basically wondering if something like this might be more clear:

/namespace=bar&name=foo

or

/bar/foo

I guess I'm a bit confused by the mixing of the path and the query params

I also think it might be worth exploring how resources are accessed in the k8s APIs, e.g.:

kubectl get pods leeroy-app-547cc64c4c-dxnkk --v=7
...
I0826 16:29:55.429372   22633 round_trippers.go:420] GET https://someip/api/v1/namespaces/default/pods/leeroy-app-547cc64c4c-dxnkk

The path to a specific pod is namespaces/default/pods/leeroy-app-547cc64c4c-dxnkk, would it make sense to follow a specific format?

@dibyom
Copy link
Member

dibyom commented Aug 26, 2020

@bobcatfish for the selectors, some discussion here: #148 (comment) and some alternatives explored in the doc: https://docs.google.com/document/d/1zWpmEhtSNe8KAPKvTJE7Pjg5Uzk8mx9MUjSN24D1jUg/edit#

re: paths and labels -> some discussion here: #148 (comment)
I think the reason query params were added were to address the scenario - how can the same webhook target more than one trigger --> which is a behavior we support today but would not with the initial path proposal which was /$namespace/$triggerName.

This is an area we could investigate more though -- and in terms of implementation plan, this is something that @khrm was targetting for v0.10 so we have some time to think through

@bobcatfish bobcatfish added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Aug 31, 2020
@bobcatfish bobcatfish changed the title Changing the TEP Status to Implementable Changing the Triggers CRD TEP Status to Implementable Aug 31, 2020
@dibyom
Copy link
Member

dibyom commented Sep 1, 2020

From API WG:

  • We should add any open questions to the TEP doc
  • Mark as implementable and proceed.
  • Come back and address open questions as we resolve them

Open Questions I can think of:

  • What kind of selectors do we need i.e do we need both namespaceSelecor and triggerSelector?
    (I think we are just going to start off with selecting on namespace names and then decide if we need to add the rest)

  • What should the path for a path based selectors look?
    (From @bobcatfish 's comment in Changing the Triggers CRD TEP Status to Implementable #184 (comment))

  • Can multiple ELs point to the same Trigger?

/cc @gabemontero @bobcatfish in case I missed something.

@bobcatfish
Copy link
Contributor

Thanks @dibyom !

/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2020
@vdemeester
Copy link
Member

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, vdemeester

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 8, 2020
Change the Status of 0009 Triggers CRD TEP Status to implementable
and added open questions
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 8, 2020
---
# TEP-0009: Introducing TriggerCRD


<!-- toc -->
- [TEP-0009: Introducing TriggerCRD](#tep-0009-introducing-triggercrd)
Copy link
Contributor Author

@khrm khrm Sep 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running ./hack/update-toc.sh script remove this line.

@dibyom
Copy link
Member

dibyom commented Sep 9, 2020

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2020
@tekton-robot tekton-robot merged commit 72cd0df into tektoncd:master Sep 9, 2020
@khrm khrm deleted the TEPStatus branch July 29, 2021 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants