init-webhook

[vincent-pli]

Changes

1. Implement webhook use knative/pkg and tektoncd/pipeline
2. Add ko related configuration files

WIP:
types depends on #17
types must implement GenericCRD

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

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[bobcatfish]

Thanks for working on this @vincent-pli !!

@vdemeester was suggesting in #18 (comment) that we look into the injection based controllers, I assume we can use that for the webhook controller too

[vincent-pli]

@bobcatfish
I checked the enjection-gen, seems it's used for simplify CRD controller implements by us.
but the webhook's controller is come from knative/pkg
need further confirmation...

[bobcatfish]

but the webhook's controller is come from knative/pkg

hm I thought you would be able to use the injection interface for webhooks too, but looking at pipelines we don't seem to be using it 🤔 https://github.com/tektoncd/pipeline/blob/196a945c2dd795b9ef5e516f365e69c69218754e/cmd/webhook/main.go#L42 (maybe @vdemeester just didn't have a chance to update it!)

@vdemeester do you know if it's possible to use the injection interface for webhook controllers too?

[vdemeester]

@bobcatfish I am looking into that 😉 (both for here and pipelines 😉 )

[vincent-pli]

@bobcatfish @vdemeester
I'm afraid the webhook can not leverage injection controller, see the issue:
knative/pkg#450

[vincent-pli]

@iancoffey
Seems we upgrade k8s.io/api from version = "kubernetes-1.12.6"(pipeline used) to version = "kubernetes-1.15.0"

that's broken the webhook implements in knative/pkg
I get the exception:

vendor/github.com/knative/pkg/webhook/webhook.go:357:15: undefined: "github.com/tektoncd/triggers/vendor/k8s.io/api/admissionregistration/v1beta1".Webhook

since the new k8s.io/api discard the Webhook, use MutatingWebhook instead.

Could we rollback to kubernetes-1.12.6

[iancoffey]

@vincent-pli Yes indeed, sounds like we may need to rollback to kubernetes-1.12.6 👍

Matching what Pipelines has defined sounds reasonable to me.

[vincent-pli]

@iancoffey
Thanks

[vincent-pli]

/retest

[ncskier]

Hi @vincent-pli, thanks for working on the webhook controller! I just saw that this PR was ready for review, so sorry that it didn't get reviewed sooner. Overall it looks really good, I just have a handful of comments/questions. 🙂

[ncskier]

I'm not entirely sure what the WithContext: field does in the pipeline webhook, but is there a reason why we don't want to include it here?

[vincent-pli]

The propose of this setting is try to put some default value in to context, I guess we still not ready for this part, anyway, I add one, just return the parameter itself.

WithContext: func(ctx context.Context) context.Context {
                        return ctx
                },

[vdemeester]

If it's not used, I think you can just skip this field altogether. It can be used to pass some value/key to tell the controller that it will need to update some version to set defaults, etc..

[ncskier]

Hi @vincent-pli, thank you for making the updates! I just have a few nits before I'm ready to approve your PR (the only one that's stopping me from approving the PR right now is the 200-clusterrole.yaml comment). 😄

[ncskier]

Thanks for making those changes @vincent-pli, looks great!
/lgtm

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncskier, vincent-pli

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 [ncskier]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[bobcatfish]

I just noticed @vincent-pli - in future we want to make sure that commit messages follow our commit message guidelines - it can be a bit hard to do while responding to feedback on a PR tho 🙏 Sometimes amending one commit as you go can be a bit easier.

[ncskier]

Sorry, I missed the commit messages when reviewing 😓

[bobcatfish]

No worries @ncskier I did too! Thanks for tolerating the nag 😜

[vincent-pli]

@bobcatfish @ncskier
Thanks for your comments, I will strict follow the guideline in the next commit, thanks again, very helpful.