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

Remove duplicated default configmaps, and stash them in core #2936

Merged

Conversation

matzew
Copy link
Member

@matzew matzew commented Apr 8, 2020

Signed-off-by: Matthias Wessendorf [email protected]

Fixes #2934

Proposed Changes

  • removes duplicated configs for both brokers.
  • making (for now) the channel-broker the default

follow up issues:

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 8, 2020
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 8, 2020
@matzew
Copy link
Member Author

matzew commented Apr 8, 2020

/test pull-knative-eventing-integration-tests

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 8, 2020
@aslom
Copy link
Member

aslom commented Apr 8, 2020

What happens when broker and mt-broker are applied, then deleted and then applied? Is config preserved? Is it documented?

@matzew
Copy link
Member Author

matzew commented Apr 8, 2020

well, as discussed in the issue, this PR will do the same behavior for brokers, that we have for channels...

and yes, there is also a reference to an issue on docs. I will move on, with doc'ing that, once this is in.

@grantr
Copy link
Contributor

grantr commented Apr 8, 2020

Haven't had a chance to review yet, but I approve of the approach. It's worked for channels so far.

@aliok
Copy link
Member

aliok commented Apr 8, 2020

Maybe out of scope for this PR, but it is related: do we need to ship the MT broker as an optional thing?

With next release eventing.yaml and eventing-mt.yaml artifacts will be created.
If you apply eventing.yaml, you won't have the MTBroker.
If you apply eventing-mt.yaml, you will have the MTBroker but it won't be the default one if this PR is merged.

In last operations WG meeting, @vaikas was saying that we want to support the scenario that both brokers exist in a Knative installation.

So, my suggestion is that we also modify this part:

["eventing-mt.yaml"]="eventing-core.yaml mt-channel-broker.yaml in-memory-channel.yaml"

so that there will be a single eventing.yaml which has both brokers. And, what the default broker will be is defined as with the config-br-defaults configmap.

@matzew
Copy link
Member Author

matzew commented Apr 8, 2020

ah, good point @aliok - I did touch that file too, now

@antoineco
Copy link
Contributor

antoineco commented Apr 8, 2020

Looks good to me!

Just one comment. Shouldn't all configurations be created under config/core/configmaps and then symlinked inside config/? I see that most other manifests are symlinked that way.

@vaikas
Copy link
Contributor

vaikas commented Apr 9, 2020

For some reason I'm unable to add this comment using github comments, but here:
config/400-config-br-defaults.yaml
L21 comment seems wrong and a carry over from somewhere else?
I'm fine with fixing later.

@vaikas
Copy link
Contributor

vaikas commented Apr 9, 2020

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, matzew, vaikas

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

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. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove default configs for the Brokers ?
8 participants