[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 21 commits into from
Nov 6, 2023
Merged

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

merged 21 commits into from
Nov 6, 2023

Conversation

matzew
Copy link
Member
@matzew matzew commented Sep 29, 2023 •
edited
Loading

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 •
edited
Loading

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.

lkingland
lkingland reviewed Oct 3, 2023
View reviewed changes
Copy link
Member
@lkingland lkingland left a comment •
edited
Loading

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 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?

@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
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
@matzew
Adding support for func subscribe for creating mutiple triggers, ba…
a7661bc 
…sed on event filters

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew @lkingland
Update cmd/subscribe.go
b059ec3 
Co-authored-by: Luke Kingland <luke@lukekingland.com>
@matzew @lkingland
Update cmd/subscribe.go
723de4a 
Co-authored-by: Luke Kingland <luke@lukekingland.com>
@matzew @lkingland
Update cmd/subscribe.go
7bab721 
Co-authored-by: Luke Kingland <luke@lukekingland.com>
@matzew @lkingland
Update cmd/subscribe.go
4321b8d 
Co-authored-by: Luke Kingland <luke@lukekingland.com>
matzew and others added 14 commits November 6, 2023 08:18
@matzew @lkingland
Update cmd/subscribe.go
fd44fc4 
Co-authored-by: Luke Kingland <luke@lukekingland.com>
@matzew @lkingland
Update cmd/subscribe.go
69eee2a 
Co-authored-by: Luke Kingland <luke@lukekingland.com>
@matzew
removing unused import
aa65ae5 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
running make
e790c86 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
Some import ogranization
84b8308 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
Change argument syntax
9c0ce9c 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
changes
bfcc517 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
Adding some emoji text
0555958 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
💄 move subscriptions underneath the deploy element
972f87f 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
adding silly emoji to build
057755f 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
Adding some simple/copied/modified test
5a23f47 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
Running 'make schema-generate'
e5f1c5d 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
Update function
1437cab 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
Little unit test
810a003 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
Adding a bit more help text
6dcacfc 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew
misspell instruction
c30a5c4 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
lkingland
lkingland approved these changes Nov 6, 2023
View reviewed changes
cmd/root.go
@@ -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
@knative-prow Knative Prow
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
@matzew @lkingland
Adding support for func subscribe for creating mutiple triggers, ba…
8781a35 
…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>
@matzew matzew mentioned this pull request Nov 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lkingland lkingland lkingland approved these changes

@jrangelramos jrangelramos Awaiting requested review from jrangelramos

@maximilien maximilien Awaiting requested review from maximilien

Assignees

@lkingland lkingland

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matzew @jrangelramos @lkingland