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

Adding support for func subscribe for creating mutiple triggers, based on event filters #2001

Merged
merged 21 commits into from
Nov 6, 2023

Conversation

matzew
Copy link
Member

@matzew matzew commented Sep 29, 2023

Little demo: https://youtu.be/O6M7mfR6JfM

Changes

    • 🎁 Add new subscribe command to create Knative Triggers for event routing

/kind

Fixes #

Release Note

kn func subscribe will allow you to create Knative Eventing Triggers for improved event processing for kn func

Docs


@knative-prow
Copy link

knative-prow bot commented Sep 29, 2023

@matzew: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

In response to this:

…sed on event filters

Changes

/kind

Fixes #

Release Note


Docs


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.

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 29, 2023
@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 29, 2023
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Attention: 65 lines in your changes are missing coverage. Please review.

Comparison is base (d258a19) 63.62% compared to head (c30a5c4) 64.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2001      +/-   ##
==========================================
+ Coverage   63.62%   64.47%   +0.85%     
==========================================
  Files         107      108       +1     
  Lines       13705    13839     +134     
==========================================
+ Hits         8720     8923     +203     
+ Misses       4129     4026     -103     
- Partials      856      890      +34     
Flag Coverage Δ
e2e-test 36.34% <25.37%> (-0.11%) ⬇️
e2e-test-oncluster 30.46% <24.62%> (+0.05%) ⬆️
e2e-test-oncluster-runtime 26.71% <19.40%> (?)
e2e-test-runtime-go 25.62% <24.62%> (?)
e2e-test-runtime-node 26.62% <24.62%> (?)
e2e-test-runtime-python 26.59% <24.62%> (?)
e2e-test-runtime-quarkus 26.73% <24.62%> (?)
e2e-test-runtime-rust 25.60% <24.62%> (?)
e2e-test-runtime-springboot 25.76% <24.62%> (?)
e2e-test-runtime-typescript 26.73% <24.62%> (?)
integration-tests 51.87% <51.49%> (+2.05%) ⬆️
unit-tests-macos-latest 48.73% <45.52%> (-0.04%) ⬇️
unit-tests-ubuntu-latest 49.48% <45.52%> (-0.02%) ⬇️
unit-tests-windows-latest 48.78% <45.52%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cmd/root.go 81.46% <100.00%> (+0.08%) ⬆️
pkg/functions/function.go 85.85% <ø> (ø)
cmd/subscribe.go 80.00% <80.00%> (ø)
pkg/knative/deployer.go 61.09% <13.79%> (-3.69%) ⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@lkingland lkingland left a comment

Choose a reason for hiding this comment

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

Looking good so far!

You'll also want to run goimports to resolve the linter error, and run make to get the updated schema file 👍🏻

cmd/subscribe.go Outdated Show resolved Hide resolved
cmd/subscribe.go Outdated Show resolved Hide resolved
cmd/subscribe.go Outdated Show resolved Hide resolved
cmd/subscribe.go Outdated Show resolved Hide resolved
cmd/subscribe.go Outdated Show resolved Hide resolved
cmd/subscribe.go Outdated Show resolved Hide resolved
pkg/knative/deployer.go Outdated Show resolved Hide resolved
@matzew matzew force-pushed the add_func_subscribe branch from 479723f to 365bb02 Compare October 10, 2023 14:04
@matzew matzew changed the title WIP: Adding support for func subscribe for creating mutiple triggers, ba… Adding support for func subscribe for creating mutiple triggers, based on event filters Oct 10, 2023
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 10, 2023
@matzew
Copy link
Member Author

matzew commented Oct 10, 2023

/hold

the filtering needs to be a bit different

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 10, 2023
@jrangelramos
Copy link
Contributor

Hi @matzew . If possible, could you add some test pls?

@matzew matzew force-pushed the add_func_subscribe branch from 555d90e to 1b85459 Compare October 11, 2023 12:38
@lkingland
Copy link
Member

Hi @matzew . If possible, could you add some test pls?

We do install Eventing in our test cluster, so an integration test in https://github.com/knative/func/blob/main/pkg/functions/client_int_test.go might be fairly straightforward

@matzew matzew force-pushed the add_func_subscribe branch 2 times, most recently from 5280d74 to cbbac15 Compare October 19, 2023 10:27
@matzew matzew force-pushed the add_func_subscribe branch 5 times, most recently from a3f81a7 to 0855979 Compare October 31, 2023 10:27
@matzew matzew force-pushed the add_func_subscribe branch from 0855979 to 1f685de Compare November 2, 2023 08:32
@matzew
Copy link
Member Author

matzew commented Nov 2, 2023

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2023
matzew and others added 5 commits November 6, 2023 08:18
…sed on event filters

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Co-authored-by: Luke Kingland <luke@lukekingland.com>
Co-authored-by: Luke Kingland <luke@lukekingland.com>
Co-authored-by: Luke Kingland <luke@lukekingland.com>
Co-authored-by: Luke Kingland <luke@lukekingland.com>
matzew and others added 14 commits November 6, 2023 08:18
Co-authored-by: Luke Kingland <luke@lukekingland.com>
Co-authored-by: Luke Kingland <luke@lukekingland.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew matzew force-pushed the add_func_subscribe branch from a4b5e99 to 810a003 Compare November 6, 2023 07:19
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew matzew force-pushed the add_func_subscribe branch from 5774349 to c30a5c4 Compare November 6, 2023 13:12
@@ -76,6 +76,7 @@ Learn more about Knative at: https://knative.dev`, cfg.Name),
NewDeployCmd(newClient),
NewDeleteCmd(newClient),
NewListCmd(newClient),
NewSubscribeCmd(),
Copy link
Member

Choose a reason for hiding this comment

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

For testing support, this should pass through the newClient client factory; but I don't think that should hold up the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add additional PR. thanks for the review on this

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2023
Copy link

knative-prow bot commented Nov 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2023
@knative-prow knative-prow bot merged commit 5bb373a into knative:main Nov 6, 2023
42 checks passed
matzew added a commit to matzew/kn-plugin-func that referenced this pull request Nov 7, 2023
…sed on event filters (knative#2001)

* Adding support for `func subscribe` for creating mutiple triggers, based on event filters

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* Update cmd/subscribe.go

Co-authored-by: Luke Kingland <luke@lukekingland.com>

* Update cmd/subscribe.go

Co-authored-by: Luke Kingland <luke@lukekingland.com>

* Update cmd/subscribe.go

Co-authored-by: Luke Kingland <luke@lukekingland.com>

* Update cmd/subscribe.go

Co-authored-by: Luke Kingland <luke@lukekingland.com>

* Update cmd/subscribe.go

Co-authored-by: Luke Kingland <luke@lukekingland.com>

* Update cmd/subscribe.go

Co-authored-by: Luke Kingland <luke@lukekingland.com>

* removing unused import

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* running make

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* Some import ogranization

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* Change argument syntax

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* changes

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* Adding some emoji text

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* 💄 move subscriptions underneath the deploy element

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* adding silly emoji to build

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* Adding some simple/copied/modified test

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* Running 'make schema-generate'

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* Update function

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* Little unit test

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* Adding a bit more help text

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

* misspell instruction

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

---------

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Co-authored-by: Luke Kingland <luke@lukekingland.com>
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. 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
Development

Successfully merging this pull request may close these issues.

3 participants