Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

kafkasource: change bootstrapServers and Topics to accept arrays #1156

Merged
merged 5 commits into from
Apr 22, 2020
Merged

kafkasource: change bootstrapServers and Topics to accept arrays #1156

merged 5 commits into from
Apr 22, 2020

Conversation

lberk
Copy link
Member

@lberk lberk commented Apr 22, 2020 •
edited
Loading

Each item in the list can be comma separated

Release Note
Action Required: KafkaSource now requires bootstrapServers and Topics to be in lists

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 22, 2020
@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 22, 2020
@knative-prow-robot knative-prow-robot added area/test-and-release and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 22, 2020
@lionelvillard
Copy link
Member

/hold

This change is not backward compatible.

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2020
@lberk
Copy link
Member Author

lberk commented Apr 22, 2020

@lionelvillard right, I'm open to suggestions on how to do so -- but keeping in mind that the kafka api is v1alpha1 we're not actually beholden (yet) to keeping it a plain string.

Keep in mind that this change allows users to keep comma separated values in each element in the list, so it would be a minimal change.

@aslom
Copy link
Member

aslom commented Apr 22, 2020

This will be backward incompatible change for kafka source in knative eventing.

Not sure about how YAML parsing is done - is it possible to support both string and sequence of strings?

@matzew
Copy link
Member

matzew commented Apr 22, 2020

I'd also not block this, due to the fact that kafka is early 🤷‍♂️

@lionelvillard
Copy link
Member

/unhold

I'm fine with just having an Action Required in the release section.

Can you also open a doc issue to update the relevant Kafka examples?

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2020
mattmoor
mattmoor reviewed Apr 22, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

kafka/source/README.md Outdated Show resolved Hide resolved
kafka/source/README.md Outdated Show resolved Hide resolved
lberk added a commit to lberk/docs that referenced this pull request Apr 22, 2020
@lberk
Update KafkaSource example with updated bootstrapServer, Topics format
555b546 
in knative/eventing-contrib#1156 we've updated the format for the
topics and bootstrapServer to be lists, update example accordingly
@lberk lberk mentioned this pull request Apr 22, 2020
Merged
@lberk
Copy link
Member Author

lberk commented Apr 22, 2020

@lionelvillard done, I've opened knative/docs#2410 to update the docs instead of filing an issue. Also updated the README.md in kafka's own source
thanks!

knative-prow-robot pushed a commit to knative/docs that referenced this pull request Apr 22, 2020
@lberk
Update KafkaSource example with updated bootstrapServer, Topics format (
478839f 
#2410)

in knative/eventing-contrib#1156 we've updated the format for the
topics and bootstrapServer to be lists, update example accordingly
@lionelvillard
Copy link
Member

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lberk, lionelvillard

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2020
@knative-prow-robot knative-prow-robot merged commit 4a32853 into knative:master Apr 22, 2020
@lberk lberk mentioned this pull request May 21, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattmoor mattmoor mattmoor left review comments

@aslom aslom Awaiting requested review from aslom

@matzew matzew Awaiting requested review from matzew

Assignees

@lionelvillard lionelvillard

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kafka source - comma or array for servers and topics
7 participants
@lberk @lionelvillard @aslom @matzew @knative-prow-robot @mattmoor @googlebot