-
Notifications
You must be signed in to change notification settings - Fork 225
Bump KafkaChannel types to v1beta1 #1356
Bump KafkaChannel types to v1beta1 #1356
Conversation
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)
if c.Annotations == nil { | ||
c.Annotations = make(map[string]string) | ||
} | ||
if _, ok := c.Annotations[messaging.SubscribableDuckVersionAnnotation]; !ok { | ||
c.Annotations[messaging.SubscribableDuckVersionAnnotation] = "v1" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this to support duckv1, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
ReplicationFactor int16 `json:"replicationFactor"` | ||
|
||
// KafkaChannel conforms to Duck type Subscribable. | ||
Subscribable *eventingduck.Subscribable `json:"subscribable,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I change this line with
eventingduckv1.ChannelableSpec `json:",inline"`
similar to InMemoryChannel v1?
We'd also have a Delivery *DeliverySpec
in that case in KafkaChannelSpec
More context: https://knative.slack.com/archives/CR2141CGY/p1593428283210700
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @n3wscott
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea and also the Status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aliok something like:
// KafkaChannelSpec defines the specification for a KafkaChannel.
type KafkaChannelSpec struct {
// NumPartitions is the number of partitions of a Kafka topic. By default, it is set to 1.
NumPartitions int32 `json:"numPartitions"`
// ReplicationFactor is the replication factor of a Kafka topic. By default, it is set to 1.
ReplicationFactor int16 `json:"replicationFactor"`
// Channel conforms to Duck type Channelable.
eventingduck.ChannelableSpec `json:",inline"`
}
// KafkaChannelStatus represents the current state of a KafkaChannel.
type KafkaChannelStatus struct {
// Channel conforms to Duck type Channelable.
eventingduck.ChannelableStatus `json:",inline"`
}
The following is the coverage report on the affected files.
|
@aliok: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
cc @lionelvillard @matzew |
/lgtm I will follow up w/ a PR for the types |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aliok, matzew 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 |
Fixes #1351
Proposed Changes
Release Note
Docs