Skip to content

Split the channel broker off from eventing core #2519

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.

Sign up for GitHub

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

Jump to bottom
Merged
knative-prow-robot merged 3 commits into from
Feb 7, 2020
Merged

Split the channel broker off from eventing core #2519

knative-prow-robot merged 3 commits into from
Feb 7, 2020

Conversation

@mattmoor
Copy link
Member

@mattmoor mattmoor commented Feb 7, 2020 •
edited
Loading 

Release the Channel-based Broker as a separate artifact from eventing-core.yaml (channel-broker.yaml)

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 7, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 7, 2020
@knative-prow-robot knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Feb 7, 2020
@mattmoor mattmoor force-pushed the split-broker-deployment branch from 4bf1070 to ec859d0 Compare February 7, 2020 00:42
@vaikas
Copy link
Contributor

vaikas commented Feb 7, 2020

/lgtm
/approve

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 7, 2020
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2020
mattmoor
mattmoor commented Feb 7, 2020
View reviewed changes
@mattmoor mattmoor changed the title [WIP] Split the channel broker off from eventing core Split the channel broker off from eventing core Feb 7, 2020
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2020
@mattmoor
Copy link
Member Author

mattmoor commented Feb 7, 2020

I'm going to start on the corresponding docs update now.

mattmoor added a commit to mattmoor/docs that referenced this pull request Feb 7, 2020
@mattmoor
Reflect the separation of the Channel-based Broker.
2dd32e5 
See also: knative/eventing#2519
@mattmoor mattmoor mentioned this pull request Feb 7, 2020
Merged
mattmoor added a commit to mattmoor/docs that referenced this pull request Feb 7, 2020
@mattmoor
Reflect the separation of the Channel-based Broker.
234d95f 
See also: knative/eventing#2519
mattmoor added a commit to mattmoor/docs that referenced this pull request Feb 7, 2020
@mattmoor
Reflect the separation of the Channel-based Broker.
fe00b17 
See also: knative/eventing#2519
@vaikas
Copy link
Contributor

vaikas commented Feb 7, 2020

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2020
@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented Feb 7, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vaikas

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 merged commit 109d948 into knative:master Feb 7, 2020
@mattmoor mattmoor deleted the split-broker-deployment branch February 7, 2020 01:43
knative-prow-robot pushed a commit to knative/docs that referenced this pull request Feb 7, 2020
@mattmoor
Reflect the separation of the Channel-based Broker. (#2182)
b59a3d9 
See also: knative/eventing#2519
@matzew
Copy link
Member

matzew commented Feb 7, 2020

Ok... why do we really need this ?

Because of mink ?

This was referenced Feb 7, 2020
Merged
Closed
grantr
grantr reviewed Feb 7, 2020
View reviewed changes
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

Like @matzew I'm curious why this is a useful change right now. Note:

  • This adds a pod to the default installation of eventing. It is not just a change to the release pipeline.
  • It doesn't improve the resource usage of installations using non-default brokers.
  • It doesn't improve the ability to develop or install non-default brokers.

@mattmoor can you update the PR description to explain your reasoning?

If there's no obvious need for this PR today, my preference is to revert it to avoid running an additional pod.

config/brokers/channel-broker/roles/controller-clusterroles.yaml
name: knative-eventing-channel-broker-controller
labels:
eventing.knative.dev/release: devel
rules:
Copy link
Contributor

@grantr grantr Feb 7, 2020

Choose a reason for hiding this comment

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

This should include all permissions the broker controller needs.

config/brokers/channel-broker/deployments/controller.yaml
app: broker-controller
eventing.knative.dev/release: devel
spec:
serviceAccountName: eventing-controller
Copy link
Contributor

@grantr grantr Feb 7, 2020

Choose a reason for hiding this comment

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

This controller should have a different service account with the permissions it needs rather than sharing with the core eventing controller.

cmd/channel_broker/main.go
)

func main() {
sharedmain.Main("controller",
Copy link
Contributor

@grantr grantr Feb 7, 2020

Choose a reason for hiding this comment

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

These controllers have accompanying webhooks that still live in the eventing-webhook deployment. Was that missed or intentional?

@matzew matzew mentioned this pull request Feb 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chaodaiG chaodaiG Awaiting requested review from chaodaiG

@nachocano nachocano Awaiting requested review from nachocano

1 more reviewer

@grantr grantr grantr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@vaikas vaikas

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mattmoor @vaikas @knative-prow-robot @matzew @grantr @googlebot