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

Add a new CRD type called Trigger #628

Merged
merged 1 commit into from
Jul 1, 2020

Conversation

dorismeixing
Copy link
Member

@dorismeixing dorismeixing commented Jun 23, 2020

Changes

/kind misc

Adds the Trigger type. It has not been wired up and cannot be used yet.

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.

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 23, 2020
@dorismeixing
Copy link
Member Author

add reviewer @dibyom

Copy link
Contributor

@khrm khrm left a comment

Choose a reason for hiding this comment

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

I feel in first iteration, we should only have TriggersCRD which can be used as ref inside the EventListener Spec.

pkg/apis/triggers/v1alpha1/trigger_types.go Show resolved Hide resolved
pkg/apis/triggers/v1alpha1/trigger_types.go Outdated Show resolved Hide resolved
@dorismeixing
Copy link
Member Author

dorismeixing commented Jun 24, 2020

/retest

@tekton-robot tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2020
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 25, 2020
Copy link
Contributor

@khrm khrm left a comment

Choose a reason for hiding this comment

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

@dibyom
Copy link
Member

dibyom commented Jun 26, 2020

/test pull-tekton-triggers-integration-tests

@dibyom
Copy link
Member

dibyom commented Jun 26, 2020

The integration test failure looks unrelated.
Once you address @khrm 's comment and squash the two commits into one, we are good to go!

config/200-clusterrole.yaml Outdated Show resolved Hide resolved
@dibyom
Copy link
Member

dibyom commented Jun 29, 2020

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2020
Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

Do we know why the data in third_party was deleted? IIRC we need this to include license information in released images via kodata

@dibyom
Copy link
Member

dibyom commented Jun 30, 2020

Do we know why the data in third_party was deleted? IIRC we need this to include license information in released images via kodata

FWIW, when I run ./hack/update-deps.sh on master, I get the same result i.e. the same license files are deleted 🙃

@dibyom
Copy link
Member

dibyom commented Jul 1, 2020

I opened #653 to track the LICENSE removal bug.

@wlynch
Copy link
Member

wlynch commented Jul 1, 2020

/lgtm
/approval

@tekton-robot tekton-robot added the kind/misc Categorizes issue or PR as a miscellaneuous one. label Jul 1, 2020
@wlynch
Copy link
Member

wlynch commented Jul 1, 2020

/approve

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wlynch

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2020
@tekton-robot tekton-robot merged commit a0d6dbe into tektoncd:master Jul 1, 2020
@savitaashture savitaashture mentioned this pull request Jul 2, 2020
3 tasks
@bobcatfish
Copy link
Collaborator

Hey @dorismeixing @wlynch @dibyom a few requests:

  • Is there a design doc and/or issue that accompanies this?
  • Can there be usage docs on how to use this?

@dorismeixing
Copy link
Member Author

Hi @bobcatfish, I think it's related to #624 and it will be used in Trigger CLI, but I am not sure if there is a design doc. I can add an usage doc if needed.

@bobcatfish
Copy link
Collaborator

Hey @dorismeixing !

I think it's related to #624 and it will be used in Trigger CLI

Looking at #624 I don't see a design doc there either 🤔 I'm a bit surprised that we'd add a new CRD without a design doc - maybe @wlynch or @dibyom when he's back have more info?

I can add an usage doc if needed.

yes plz :D usually we try to accompany every commit with the related docs changes (https://github.com/tektoncd/community/blob/master/standards.md#principles), so since we've added a new type here I'd expect to see some docs at https://github.com/tektoncd/triggers/tree/master/docs about how to use it

@dibyom
Copy link
Member

dibyom commented Jul 20, 2020

Triggers WG has been discussing a Trigger CRD in https://docs.google.com/document/d/1NX0ExhPad6ixTM8AdU0b6Vc3MVD5hQ_vIrOs9dIXq-I/edit# and https://docs.google.com/document/d/1zWpmEhtSNe8KAPKvTJE7Pjg5Uzk8mx9MUjSN24D1jUg/edit#

We reached consensus on a Trigger CRD + selector approach in the Triggers WG and a TEP has been filed with the proposed approach: tektoncd/community#148

This PR adds the type but it is not ready to be used by an end user yet. The main motivation for adding the type now was to simplify the run CLI tool while we worked out the open questions/TEP process for the overall Trigger/Selector approach. The CRD will be usable once we either have the run CLI implemented or we implement the approach from the TEP.

(One thing is that we haven't been very good at updating the GitHub issues with the status of our discussion from the WGs/docs -- I'll update the issues)

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/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants