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

feature(source binding): Support for Sink Binding as source #625

Merged
merged 8 commits into from
Jan 29, 2020

Conversation

rhuss
Copy link
Contributor

@rhuss rhuss commented Jan 27, 2020

Sink bindings are managed like any other source. Sinks are specified as usual (with prefix and name),
'subjects' (the other end of the binding) is managed via a shortcut notation:

  • with name: <kind>:<apiVersion>:<name>
  • with label selector: <kind>:<apiVersion>:key1=value1,key2=value2

With --subject-namespace and additional namespace can be provided (shoudl be possible for a sink, too but is not yet) As discussed on Slack, neither for a sink nor a subject the namespace is setable by the user. So I removed that option.

The implementation already uses the new sink binding from the sources.knative.dev group
and hence is a bit inconsistent to the still old usage kf sources.eventing.knative.dev for apiserver source and cronjob.

However as we will move over to sources.knative.dev very soon (right after v0.12.0) release,
this is was more appropriates.

Still WIP, but eventually fixes #624

Task list:

  • create
  • update
  • delete
  • describe
  • list

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 27, 2020
@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 27, 2020
@rhuss rhuss added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 27, 2020
@rhuss
Copy link
Contributor Author

rhuss commented Jan 27, 2020

One question to the community: I would like to include this feature into 0.12.0 which is due tomorrow. However, because of time constraints and outstanding review, I would suggest to postpone the release to Wednesday (and I would some help with some quick reviewers ;-)

wdyt ? We can discuss this tomorrown on the WG call, however as I'm on a train at that time, not sure how it will work out for me.

// cc: @sixolet @maximilien @navidshaikh

@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 27, 2020
@rhuss rhuss added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2020
rhuss added 4 commits January 28, 2020 15:49
Sink bindings are managed like any other source. Sinks are specified as usual (with prefix and name),
'subjects' (the other end of the binding) is managed via a shortcut notation:

* with name: `<kind>:<apiVersion>:<name>`
* with label selector: `<kind>:<apiVersion>:key1=value1,key2=value2`

With `--subject-namespace` and additional namespace can be provided (shoudl be possible for a sink, too but is not yet)

The implementation already uses the new sink binding from the `sources.knative.dev` group
and hence is a bit inconsistent to the still old usage kf `sources.eventing.knative.dev` for apiserver source and cronjob.

However as we will move over to `sources.knative.dev` very soon (right after v0.12.0) release,
this is was more appropriates.

Still WIP, but eventually fixes knative#624

Task list:

- [X] create
- [] update
- [] delete
- [] describe
- [] list
Also, it looks like that knative eventing 0.12.0 is still on the old api group.
So I need to move this code to use the legacyclient, too, for the sink binding
and only switch over to the new client for 0.13.0
* update
* delete
* list
* describe
@rhuss rhuss removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2020
@rhuss
Copy link
Contributor Author

rhuss commented Jan 28, 2020

ready for review

Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

Did not manually test. Left various small comments. LGTM

docs/cmd/kn_source_binding.md Outdated Show resolved Hide resolved
pkg/kn/commands/source/binding/binding_test.go Outdated Show resolved Hide resolved
pkg/kn/commands/source/binding/create.go Outdated Show resolved Hide resolved
pkg/kn/commands/source/binding/describe.go Outdated Show resolved Hide resolved
pkg/kn/commands/source/binding/describe.go Show resolved Hide resolved
pkg/kn/commands/source/binding/describe_test.go Outdated Show resolved Hide resolved
pkg/kn/commands/source/binding/flags.go Outdated Show resolved Hide resolved
pkg/kn/commands/source/binding/flags.go Outdated Show resolved Hide resolved
pkg/kn/commands/source/binding/list.go Show resolved Hide resolved
pkg/sources/v1alpha1/binding_client_mock_test.go Outdated Show resolved Hide resolved
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, rhuss

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

pkg/eventing/legacysources/v1alpha1/client.go Outdated Show resolved Hide resolved
pkg/kn/commands/source/binding/binding.go Show resolved Hide resolved
pkg/kn/commands/source/binding/binding.go Show resolved Hide resolved
pkg/kn/commands/source/binding/binding.go Outdated Show resolved Hide resolved
pkg/kn/commands/source/binding/binding.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

@navidshaikh @maximilien just worked on the comments, thanks for the feedback ! Very helpful.

Going to push in a second with most of the comments fixed. Some are discussable but I'm sure we can resolved those, too.

@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/kn/commands/flags/sink.go 78.3% 79.2% 0.9
pkg/kn/commands/source/binding/binding.go Do not exist 58.8%
pkg/kn/commands/source/binding/create.go Do not exist 81.6%
pkg/kn/commands/source/binding/delete.go Do not exist 85.7%
pkg/kn/commands/source/binding/describe.go Do not exist 90.8%
pkg/kn/commands/source/binding/flags.go Do not exist 91.3%
pkg/kn/commands/source/binding/list.go Do not exist 78.3%
pkg/kn/commands/source/binding/update.go Do not exist 74.4%
pkg/sources/v1alpha1/binding_client.go Do not exist 85.9%
pkg/sources/v1alpha1/binding_client_mock.go Do not exist 90.9%
pkg/sources/v1alpha1/client.go Do not exist 100.0%

@navidshaikh
Copy link
Collaborator

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2020
@knative-prow-robot knative-prow-robot merged commit 164cb5f into knative:master Jan 29, 2020
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. 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.

Add kn source binding ... command
6 participants