Bringing ContainerSource back #2824

nachocano · 2020-03-24T00:20:59Z

Proposed Changes

🎁Bringing back ContainerSource in the new sources.knative.dev API group and as v1alpha2.
🧽Removed old deprecated fields and just using spec.template. Updated types to be a valid duckv1.Source. Updated validation, defaults, reconciler to use genReconciler, and some other cleanups.
Implemented it using SinkBindings as suggested by @n3wscott and @mattmoor
For more context, see Bring back ContainerSource? #2739

Release Note

ContainerSource is available in a new API group `sources.knative.dev` and as part of `v1alpha2` version. Its deprecated fields have been removed. You should use `spec.template` to configure it. Please look at the updated docs in knative.dev to see examples of how to use it. If you want/need more advanced functionality, you should resort to SinkBinding (also in the docs).
Action Required: you should migrate your ContainerSources to this new version, and delete the old CRD called `containersources.sources.eventing.knative.dev` with `kubectl delete crds containersources.sources.eventing.knative.dev`.

Docs

knative/docs#2332

/cc @lionelvillard @grantr @vaikas @n3wscott

config/300-containersource.yaml

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

config/300-containersource.yaml

nachocano · 2020-03-24T00:41:54Z

/retest

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

nachocano · 2020-03-24T01:07:25Z

/retest

…

On Mon, Mar 23, 2020 at 5:54 PM Knative Prow Robot ***@***.***> wrote: @nachocano <https://github.com/nachocano>: The following tests *failed*, say /retest to rerun all failed tests: Test name Commit Details Rerun command pull-knative-eventing-unit-tests f7452f4 <f7452f4> link <https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_eventing/2824/pull-knative-eventing-unit-tests/1242245049702420484> /test pull-knative-eventing-unit-tests pull-knative-eventing-go-coverage 34635cc <34635cc> link <https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_eventing/2824/pull-knative-eventing-go-coverage/1242252221354086402> /test pull-knative-eventing-go-coverage Full PR test history <https://gubernator.knative.dev/pr/knative_eventing/2824>. Your PR dashboard <https://gubernator.knative.dev/pr/nachocano>. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2824 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABD65DALIOAU4C7LLO7V44DRJAAELANCNFSM4LSIRB3Q> .

n3wscott · 2020-03-24T01:12:18Z

I think we need to have agreement before this can just land.

/approve cancel

nachocano · 2020-03-24T01:46:14Z

I think we need to have agreement before this can just land.

/approve cancel

@n3wscott the agreement was documented here: #2739.
2/3 eventing WG leads voted for this, plus plenty of other members in the community. Let's not start these discussions all over again. Is a waste of time for everybody. We have better things to do.
I know you disagree, but I'd really expect you to commit, so that we can move forward.
If you worry about not having enough contributors to maintain it (IIRC that was the last argument against adding this back), then don't, I'm willing to do so. And I think I've already shown that I do contribute #1, #2.

BTW nothing is set in stone, if we later on decide this is useless, we can certainly revisit.

/fyi @grantr @lionelvillard @vaikas as WG leads

n3wscott · 2020-03-24T02:44:12Z

at the very least this needs to use sink binding internally.

mattmoor · 2020-03-24T02:48:06Z

nit: can we call it DeploymentSource to better align with the syntactic sugar it represents?

fwiw I think Serving's Service resource is useful precedent for a new abstraction to simplify the resource model 🤷‍♂

n3wscott · 2020-03-24T15:39:26Z

I think we need to have agreement before this can just land.
/approve cancel

@n3wscott the agreement was documented here: #2739.
2/3 eventing WG leads voted for this, plus plenty of other members in the community. Let's not start these discussions all over again. Is a waste of time for everybody. We have better things to do.
I know you disagree, but I'd really expect you to commit, so that we can move forward.
If you worry about not having enough contributors to maintain it (IIRC that was the last argument against adding this back), then don't, I'm willing to do so. And I think I've already shown that I do contribute #1, #2.

BTW nothing is set in stone, if we later on decide this is useless, we can certainly revisit.

/fyi @grantr @lionelvillard @vaikas as WG leads

Because this is a source, it should go to the sources WG. That is the point of having a Sources WG.

vaikas · 2020-03-24T16:02:05Z

Getting the container source back is fine with me as clearly there's a need from the users/developers to make it super trivial to create sources. However, before we move too much forward with some of the proposed changes that I saw on the issue, I think we need a further discussion about some of the proposed features. For example imho DLQ does not belong imho to the source, just like fanout doesn't belong there and so forth. There should be a clean / clear separation of concerns and baking more things into what a source is seems wrong to me. We should discuss that in the context of Sources WG.

nachocano · 2020-03-24T17:10:05Z

at the very least this needs to use sink binding internally.

Maybe that can be done in a follow up. I'm not entirely convinced that is the right approach, it will create an IMO unnecessary extra object.

nachocano · 2020-03-24T17:14:05Z

nit: can we call it DeploymentSource to better align with the syntactic sugar it represents?

The implementation might change in the near future, so we'd rather not couple it with Deployments. ContainerSource is a (widely) popular name already, I think we should keep it.

fwiw I think Serving's Service resource is useful precedent for a new abstraction to simplify the resource model 🤷‍♂

@mattmoor just to be sure, you consider Serving's Service is a precedent in favor of having this ContainerSource abstraction?

nachocano · 2020-03-24T17:18:04Z

Getting the container source back is fine with me as clearly there's a need from the users/developers to make it super trivial to create sources. However, before we move too much forward with some of the proposed changes that I saw on the issue, I think we need a further discussion about some of the proposed features. For example imho DLQ does not belong imho to the source, just like fanout doesn't belong there and so forth. There should be a clean / clear separation of concerns and baking more things into what a source is seems wrong to me. We should discuss that in the context of Sources WG.

Yes, I wasn't planning on adding any of those extra bits yet, they are not included in this PR at all.
It's a fair and valid point that should require a more thorough discussion in the Sources WG.

n3wscott · 2020-03-24T18:37:53Z

at the very least this needs to use sink binding internally.

Maybe that can be done in a follow up. I'm not entirely convinced that is the right approach, it will create an IMO unnecessary extra object.

If you do not use SinkBinding, ContainerSource is duplicating all the logic of SinkBinding. So that is not ideal.

nachocano · 2020-03-24T19:41:30Z

at the very least this needs to use sink binding internally.

Maybe that can be done in a follow up. I'm not entirely convinced that is the right approach, it will create an IMO unnecessary extra object.

If you do not use SinkBinding, ContainerSource is duplicating all the logic of SinkBinding. So that is not ideal.

hmm, I think we are just doing the same for ceOverrides serialization and setting the env variables in the containers. That can be easily moved out to some util. I wonder if the ceOverrides marshalling thing is actually better to have it in pkg so that different sources can easily leverage it?

nachocano · 2020-03-26T18:59:03Z

Implemented it using SinkBinding as suggested by folks. Updated UTs, etc...

/assign @n3wscott

n3wscott · 2020-03-26T20:19:19Z

cmd/webhook/main.go

@@ -86,6 +86,7 @@ var ourTypes = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
 	sourcesv1alpha1.SchemeGroupVersion.WithKind("ApiServerSource"): &sourcesv1alpha1.ApiServerSource{},
 	sourcesv1alpha1.SchemeGroupVersion.WithKind("PingSource"):      &sourcesv1alpha1.PingSource{},
 	sourcesv1alpha1.SchemeGroupVersion.WithKind("SinkBinding"):     &sourcesv1alpha1.SinkBinding{},
+	sourcesv1alpha1.SchemeGroupVersion.WithKind("ContainerSource"): &sourcesv1alpha1.ContainerSource{},


sounds good, will do

n3wscott · 2020-03-26T20:19:44Z

config/core/resources/containersource.yaml

+      type: date
+      JSONPath: .metadata.creationTimestamp
+  versions:
+  - name: v1alpha1


ack! will do

n3wscott

lets move it to v1alpha2 and I will approve

nachocano · 2020-03-26T21:13:07Z

lets move it to v1alpha2 and I will approve

Perfect, will do!

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

pkg/reconciler/containersource/containersource.go

config/300-containersource.yaml

knative-metrics-robot · 2020-03-26T22:30:08Z

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

File	Old Coverage	New Coverage
pkg/apis/sources/v1alpha2/container_defaults.go	Do not exist	100.0%
pkg/apis/sources/v1alpha2/container_lifecycle.go	Do not exist	83.3%
pkg/apis/sources/v1alpha2/container_types.go	Do not exist	0.0%
pkg/apis/sources/v1alpha2/container_validation.go	Do not exist	88.2%
pkg/reconciler/containersource/containersource.go	Do not exist	74.1%
pkg/reconciler/containersource/controller.go	Do not exist	100.0%
pkg/reconciler/containersource/resources/deployment.go	Do not exist	100.0%
pkg/reconciler/containersource/resources/labels.go	Do not exist	100.0%
pkg/reconciler/containersource/resources/names.go	Do not exist	100.0%
pkg/reconciler/containersource/resources/sinkbinding.go	Do not exist	100.0%

knative-prow-robot · 2020-03-26T22:30:25Z

@nachocano: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-eventing-go-coverage	`73fdde6`	link	`/test pull-knative-eventing-go-coverage`

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.

n3wscott · 2020-03-26T22:30:38Z

/lgtm
/approve

🙃

knative-prow-robot · 2020-03-26T22:30:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott, nachocano

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~~ [n3wscott,nachocano]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nachocano · 2020-03-26T22:32:36Z

/lgtm
/approve

Highly appreciated @n3wscott !!!

adding ContainerSource back

f7452f4

knative-prow-robot requested review from grantr, lionelvillard, n3wscott and vaikas March 24, 2020 00:21

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 24, 2020

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 24, 2020

mattmoor reviewed Mar 24, 2020

View reviewed changes

config/300-containersource.yaml Show resolved Hide resolved

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release labels Mar 24, 2020

mattmoor reviewed Mar 24, 2020

View reviewed changes

n3wscott reviewed Mar 24, 2020

View reviewed changes

config/300-containersource.yaml Show resolved Hide resolved

remove omitempty

4b58a56

mattmoor reviewed Mar 24, 2020

View reviewed changes

lint

34635cc

mattmoor reviewed Mar 24, 2020

View reviewed changes

knative-prow-robot assigned n3wscott Mar 26, 2020

update codegen

ead139e

mattmoor reviewed Mar 26, 2020

View reviewed changes

n3wscott reviewed Mar 26, 2020

View reviewed changes

nachocano and others added 6 commits March 26, 2020 14:25

moving to v1alpha2

aa1f14f

update code gen

9e1bd55

moving to v1alpha2

1dcc641

update codegen

29a0eaf

one more try

d5f7995

fixing e2e

7668afd

mattmoor reviewed Mar 26, 2020

View reviewed changes

pkg/reconciler/containersource/containersource.go Show resolved Hide resolved

lint

c8df7d9

mattmoor reviewed Mar 26, 2020

View reviewed changes

nachocano added 2 commits March 26, 2020 15:25

fix e2e

a0a0ae7

comment

73fdde6

mattmoor reviewed Mar 26, 2020

View reviewed changes

config/300-containersource.yaml Show resolved Hide resolved

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2020

knative-prow-robot merged commit e51b8a4 into knative:master Mar 26, 2020

rhuss mentioned this pull request Mar 27, 2020

kn source container - manage ContainerSource knative/client#530

Closed

5 tasks

matzew mentioned this pull request May 6, 2020

adding deprecated label to "legacy" sources openshift-knative/eventing-operator#22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bringing ContainerSource back #2824

Bringing ContainerSource back #2824

nachocano commented Mar 24, 2020 •

edited

Loading

mattmoor left a comment

nachocano commented Mar 24, 2020

mattmoor left a comment

nachocano commented Mar 24, 2020 via email

n3wscott commented Mar 24, 2020

nachocano commented Mar 24, 2020

n3wscott commented Mar 24, 2020

mattmoor commented Mar 24, 2020

n3wscott commented Mar 24, 2020

vaikas commented Mar 24, 2020

nachocano commented Mar 24, 2020

nachocano commented Mar 24, 2020

nachocano commented Mar 24, 2020

n3wscott commented Mar 24, 2020

nachocano commented Mar 24, 2020

nachocano commented Mar 26, 2020

n3wscott Mar 26, 2020

nachocano Mar 26, 2020

n3wscott Mar 26, 2020

nachocano Mar 26, 2020

n3wscott left a comment

nachocano commented Mar 26, 2020

mattmoor left a comment

knative-metrics-robot commented Mar 26, 2020

knative-prow-robot commented Mar 26, 2020 •

edited

Loading

n3wscott commented Mar 26, 2020

knative-prow-robot commented Mar 26, 2020

nachocano commented Mar 26, 2020

Bringing ContainerSource back #2824

Bringing ContainerSource back #2824

Conversation

nachocano commented Mar 24, 2020 • edited Loading

Proposed Changes

mattmoor left a comment

Choose a reason for hiding this comment

nachocano commented Mar 24, 2020

mattmoor left a comment

Choose a reason for hiding this comment

nachocano commented Mar 24, 2020 via email

n3wscott commented Mar 24, 2020

nachocano commented Mar 24, 2020

n3wscott commented Mar 24, 2020

mattmoor commented Mar 24, 2020

n3wscott commented Mar 24, 2020

vaikas commented Mar 24, 2020

nachocano commented Mar 24, 2020

nachocano commented Mar 24, 2020

nachocano commented Mar 24, 2020

n3wscott commented Mar 24, 2020

nachocano commented Mar 24, 2020

nachocano commented Mar 26, 2020

n3wscott Mar 26, 2020

Choose a reason for hiding this comment

nachocano Mar 26, 2020

Choose a reason for hiding this comment

n3wscott Mar 26, 2020

Choose a reason for hiding this comment

nachocano Mar 26, 2020

Choose a reason for hiding this comment

n3wscott left a comment

Choose a reason for hiding this comment

nachocano commented Mar 26, 2020

mattmoor left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Mar 26, 2020

knative-prow-robot commented Mar 26, 2020 • edited Loading

n3wscott commented Mar 26, 2020

knative-prow-robot commented Mar 26, 2020

nachocano commented Mar 26, 2020

nachocano commented Mar 24, 2020 •

edited

Loading

knative-prow-robot commented Mar 26, 2020 •

edited

Loading