Skip to content

Add control plane services for Kafka Channel Provisioner#573

Merged
knative-prow-robot merged 2 commits into
knative:masterfrom
neosab:kafka_provisioner_svc
Nov 12, 2018
Merged

Add control plane services for Kafka Channel Provisioner#573
knative-prow-robot merged 2 commits into
knative:masterfrom
neosab:kafka_provisioner_svc

Conversation

@neosab
Copy link
Copy Markdown
Contributor

@neosab neosab commented Oct 31, 2018

Fixes #442

Proposed Changes

  • Adds the K8S Service & Virtual Service for the Channel
  • Adds Dispatcher Service for the kafka ClusterChannelProvisioner

I believe this would finish the required control plane setup for Kafka channel provisioner.

Release Note

@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 31, 2018
@googlebot
Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot googlebot added the cla: no Indicates the PR's author has not signed the CLA. label Oct 31, 2018
@neosab
Copy link
Copy Markdown
Contributor Author

neosab commented Oct 31, 2018

/cc @adamharwayne @n3wscott @matzew

@knative-prow-robot
Copy link
Copy Markdown
Contributor

@neosab: GitHub didn't allow me to request PR reviews from the following users: matzew.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @adamharwayne @n3wscott @matzew

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.

@sebgoa
Copy link
Copy Markdown

sebgoa commented Nov 7, 2018

there are a lot of howto/example like things that could live better in the docs repo IMHO.

@matzew
Copy link
Copy Markdown
Member

matzew commented Nov 7, 2018

@sebgoa you mean the three README files ?

we did follow the "old" model, and also the in-mem channel provisioner has some doc.

That said, once this is merged we will get also a hand on proper docs for eventing. with different chapters for different channels and sources.

IMO for the moment they live well here (or in the eventing-sources repo, e.g. see the k8sevent source demo/docs)

@sebgoa
Copy link
Copy Markdown

sebgoa commented Nov 8, 2018

@sebgoa you mean the three README files ?

we did follow the "old" model, and also the in-mem channel provisioner has some doc.

I mean everything in /config/provisioners/kafka/broker and /config/provisioners/kafka/strimzi

These are not related to the provisioner, they are two ways to deploy Kakfa in k8s. I know you need a working Kafka but these are very much docs that would better live outside this code base. Either /knative/docs or simply make a reference to the upstream documentation of these projects.

Why ? as soon as these projects do an update the documentation/instructions that you are putting in here are going to be obsolete.

Also there are many different ways to deploy Kafka on k8s. Instead of strimzi, why not the confluent operator. IMHO, To avoid contention, I would simply remove it or point to the upstream docs.

@matzew
Copy link
Copy Markdown
Member

matzew commented Nov 8, 2018 via email

@neosab neosab force-pushed the kafka_provisioner_svc branch from 7546073 to 2cbcc57 Compare November 9, 2018 09:27
@knative-prow-robot knative-prow-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 Nov 9, 2018
Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>
@neosab neosab force-pushed the kafka_provisioner_svc branch from 2cbcc57 to 27e8ea4 Compare November 9, 2018 09:49
@neosab
Copy link
Copy Markdown
Contributor Author

neosab commented Nov 9, 2018

I am fine with co-authoring the PR with @matzew

@matzew
Copy link
Copy Markdown
Member

matzew commented Nov 9, 2018 via email

@neosab
Copy link
Copy Markdown
Contributor Author

neosab commented Nov 9, 2018

/assign @evankanderson

@evankanderson
Copy link
Copy Markdown
Member

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2018
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, neosab

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2018
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2018
@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/provisioners/kafka/controller/channel/reconcile.go 84.2% 79.4% -4.8
pkg/provisioners/kafka/controller/reconcile.go 86.7% 78.9% -7.7

@neosab
Copy link
Copy Markdown
Contributor Author

neosab commented Nov 10, 2018

@evankanderson Sorry need someone to reset the CLA again

@evankanderson evankanderson added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Nov 10, 2018
@googlebot
Copy link
Copy Markdown

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@matzew
Copy link
Copy Markdown
Member

matzew commented Nov 12, 2018

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2018
@knative-prow-robot knative-prow-robot merged commit 0ad6cac into knative:master Nov 12, 2018
@grantr grantr added this to the v0.2.1 milestone Nov 28, 2018
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. cla: yes Indicates the PR's author has signed the CLA. 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.

8 participants