Skip to content
This repository has been archived by the owner on Sep 2, 2024. It is now read-only.

Add Kafka Source defaults, starting with KEDA annotations #855

Conversation

lionelvillard
Copy link
Contributor

@lionelvillard lionelvillard commented Sep 3, 2021

Proposed Changes

  • Add new config-kafka-source-defaults configmap to be used by the defaulting mutating webhook
  • When autoscaler is enabled in the configmap, the defaulting webhook automatically adds KEDA annotations.

Release Note

- 🎁Autoscaling annotations can now be automatically added to KafkaSource objects. See the documentation for more details. 

Docs
See knative/docs#4181

/cc @steven0711dong @matzew @zroubalik

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 3, 2021
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 3, 2021
@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/apis/sources/config/kafka_defaults.go Do not exist 84.2%
pkg/apis/sources/config/store.go Do not exist 93.8%

@codecov
Copy link

codecov bot commented Sep 3, 2021

Codecov Report

Merging #855 (a295d55) into main (75b06f9) will decrease coverage by 0.50%.
The diff coverage is 81.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #855      +/-   ##
==========================================
- Coverage   75.14%   74.64%   -0.51%     
==========================================
  Files         139      143       +4     
  Lines        6442     6633     +191     
==========================================
+ Hits         4841     4951     +110     
- Misses       1369     1435      +66     
- Partials      232      247      +15     
Impacted Files Coverage Δ
pkg/apis/sources/config/kafka_defaults.go 68.42% <68.42%> (ø)
pkg/apis/sources/config/store.go 92.00% <92.00%> (ø)
pkg/apis/sources/v1beta1/kafka_defaults.go 100.00% <100.00%> (ø)
pkg/source/adapter/adapter.go 54.73% <0.00%> (-3.70%) ⬇️
...rce/reconciler/source/resources/receive_adapter.go 96.51% <0.00%> (-3.49%) ⬇️
pkg/source/adapter/message.go 72.72% <0.00%> (-1.60%) ⬇️
pkg/source/mtadapter/adapter.go 46.61% <0.00%> (-1.22%) ⬇️
cmd/channel/post-install/main.go 0.00% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75b06f9...a295d55. Read the comment docs.

@steven0711dong
Copy link
Contributor

Was this tested? I just deployed this change into pg1 and created a Kafka source from there. The annotations were not added.

@lionelvillard
Copy link
Contributor Author

Was this tested? I just deployed this change into pg1 and created a Kafka source from there. The annotations were not added.

yes. What is pg1 :-) ?

Anyway, did you change the configmap to enable annotations injection?

Copy link
Contributor

@steven0711dong steven0711dong left a comment

Choose a reason for hiding this comment

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

lgtm

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

/lgtm

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [lionelvillard,steven0711dong]

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 15bb0b1 into knative-extensions:main Sep 7, 2021
@lionelvillard
Copy link
Contributor Author

/cherry-pick release-0.25

@knative-prow-robot
Copy link
Contributor

@lionelvillard: #855 failed to apply on top of branch "release-0.25":

Applying: add Kafka defaults, starting with KEDA annotations
.git/rebase-apply/patch:998: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	pkg/apis/sources/v1beta1/kafka_defaults.go
M	pkg/apis/sources/v1beta1/kafka_defaults_test.go
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
Auto-merging pkg/apis/sources/v1beta1/kafka_defaults_test.go
CONFLICT (content): Merge conflict in pkg/apis/sources/v1beta1/kafka_defaults_test.go
Auto-merging pkg/apis/sources/v1beta1/kafka_defaults.go
CONFLICT (content): Merge conflict in pkg/apis/sources/v1beta1/kafka_defaults.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 add Kafka defaults, starting with KEDA annotations
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.25

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.

lionelvillard added a commit to lionelvillard/eventing-kafka that referenced this pull request Sep 8, 2021
knative-prow-robot pushed a commit that referenced this pull request Sep 9, 2021
Co-authored-by: Steven DOng <steven.dong@ibm.com>

Co-authored-by: Steven DOng <steven.dong@ibm.com>
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants