-
Notifications
You must be signed in to change notification settings - Fork 419
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
Create a skeleton controller #18
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncskier 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 |
c027e54
to
c4dd758
Compare
@ncskier might be worth exploring injection-based controllers (see tektoncd/pipeline#1015 as a reference) |
might as well switch to the injection based controller right at the outset if we can! (sorry @ncskier 😅 ) |
I'll take a look 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looking good to me! +1 to what @vdemeester said about using the injection based controller tho if we can (might as well start off on the right foot)
(thanks @vdemeester !!)
325cc3c
to
459f938
Compare
@bobcatfish @vdemeester I've updated the controller to use the knative/pkg injection infrastructure. Please take another look at reviewing this PR when you get the chance 🙂 I only added a reconciler for the I also changed the bash scripts to use |
931e2fd
to
6adc2ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call catching that test-infra vs. plumbing change! I never would have caught that haha XD
I put in a couple questions but nothing that should stop this from getting merged imo!
Thanks @ncskier :D
/lgtm
/meow space
- pkg/client/clientset/versioned/fake | ||
linters-settings: | ||
errcheck: | ||
exclude: .errcheck.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooo what's this file for? ive never seen this before! :D
(question about both the .golangci.yaml and the .errcheck.txt!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the config file for our linter golangci-lint
(more info here). .errcheck.txt
lists files to exclude from testing. However, I don't think we need .errcheck.txt
anymore, because the linter error we were skipping was fixed the correct way in @vincent-pli's webhook PR 😅
I'm going to remove the .errcheck.txt
file in a PR that I'm putting together, because I don't think we need it anymore.
name: config-logging-triggers | ||
namespace: tekton-pipelines | ||
data: | ||
# Common configuration for all knative codebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment out of date haha
|
||
# Log level overrides | ||
loglevel.controller: "info" | ||
loglevel.webhook: "info" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like the level is already at "info", i wonder why these are explicitly also setting info 🤔
apiVersion: v1 | ||
kind: ConfigMap | ||
metadata: | ||
name: config-observability |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, is this configuring functionality we get out of the box with knative/pkg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so... @vdemeester do you know about the config-observability
functionality? 😄
@@ -1,6 +1,6 @@ | |||
#!/usr/bin/env bash | |||
|
|||
# Copyright 2018 The Knative Authors | |||
# Copyright 2019 The Tekton Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hee hee
In response to this:
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. |
Changes
Create the skeleton controller described in issue #8.
eventlistener
,triggertemplate
, andtriggerbinding
ko apply -f config/
.ko apply -f config/
hack/update-deps.sh
script and a ton ofvendor/
dependencies#### WIP:I ran into some issues withdep
, discussed on Slack here. The code in this PR builds correctly right now, but I would like to get some feedback about this issue.I've added extra constraints to the
Gopkg.toml
file to solve this issue.There is placeholder code commented out right now that depends on the generated clientset & informer code that is being added under PR Create new types and add initial generated code #17.The PR has been merged, and I created a reconciler for the
EventListener
.Thetriggers
controller Deployment uses thetekton-pipelines-controller
ServiceAccount. Under issue Make instructions in DEVELOPMENT guide work #7, this can be changed to a triggers ServiceAccount if we do not want to depend onpipelines
being installed.With PR Add config yaml files #23 we use our own
triggers
Service Account.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.