Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

Delete moved ContainerSource from eventing-sources. #371

Merged
merged 8 commits into from
May 1, 2019
Merged

Delete moved ContainerSource from eventing-sources. #371

merged 8 commits into from
May 1, 2019

Conversation

n3wscott
Copy link
Contributor

Follow-up to knative/eventing#1099

Proposed Changes

  • Delete moved ContainerSource

Release Note

ContainerSource now comes from installing eventing.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 25, 2019
@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 Apr 25, 2019
@n3wscott n3wscott mentioned this pull request Apr 25, 2019
Closed
@n3wscott
Copy link
Contributor Author

Samples will be moved as a follow-up.

@n3wscott
Copy link
Contributor Author

Related to knative/docs#1235

@n3wscott
Copy link
Contributor Author

/retest

@vaikas
Copy link
Contributor

vaikas commented Apr 25, 2019

Please also update Docs in knative/docs to reflect this. Those samples will also need to change.

@n3wscott
Copy link
Contributor Author

Please also update Docs in knative/docs to reflect this. Those samples will also need to change.

that work is being tracked here: knative/docs#1235

@n3wscott
Copy link
Contributor Author

error:

(╯°□°)╯︵  kubectl  logs -n knative-sources controller-manager-0
2019/04/25 22:56:57 Registering Components.
2019/04/25 22:56:57 Skipping the AWS SQS Source controller.
2019/04/25 22:56:57 no kind is registered for the type v1alpha1.ContainerSource in scheme "k8s.io/client-go/kubernetes/scheme/register.go:60"

Will fix.

@n3wscott
Copy link
Contributor Author

Fixed.

@n3wscott
Copy link
Contributor Author

/retest

vaikas
vaikas reviewed Apr 26, 2019
View reviewed changes
config/201-clusterrole.yaml
@@ -34,12 +33,20 @@ rules:
- patch
- delete

# ContainerSource from eventing
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

@n3wscott n3wscott Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Because there are sources that are built ontop of the ContainerSource, this allows things like the current K8s source to make ContainerSources

contrib/kafka/pkg/reconciler/kafkasource_test.go
@@ -65,7 +65,7 @@ func init() {
v1.AddToScheme(scheme.Scheme)
corev1.AddToScheme(scheme.Scheme)
sourcesv1alpha1.SchemeBuilder.AddToScheme(scheme.Scheme)
genericv1alpha1.SchemeBuilder.AddToScheme(scheme.Scheme)
eventingsourcesv1alpha1.SchemeBuilder.AddToScheme(scheme.Scheme)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just use aws here and kafka in aws so we don't have to pull in eventingsources, or is there another reason for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it needs ContainerSource from eventing

@n3wscott
Copy link
Contributor Author

I wonder... I bet this needs to install the latest version of eventing, but I bet it is installing the latest release....

@n3wscott
Copy link
Contributor Author

/retest

@n3wscott
Copy link
Contributor Author

but I bet it is installing the latest release....

it is installing the latest nightly release.

@n3wscott
Copy link
Contributor Author

/retest

1 similar comment
@n3wscott
Copy link
Contributor Author

/retest

@n3wscott
Copy link
Contributor Author

looks like it is back to the same error message.

/retest

@n3wscott
Copy link
Contributor Author

I think the there is an issue with the upstream container source. Let me go fix...

@n3wscott
Copy link
Contributor Author

Verified it works in eventing.

@vaikas
Copy link
Contributor

vaikas commented May 1, 2019

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 1, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott, vaikas-google

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,vaikas-google]

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 b3559a8 into knative:master May 1, 2019
@n3wscott n3wscott mentioned this pull request May 1, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vaikas vaikas vaikas left review comments

@evankanderson evankanderson Awaiting requested review from evankanderson

Assignees

@vaikas vaikas

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

Successfully merging this pull request may close these issues.
4 participants
@n3wscott @vaikas @knative-prow-robot @googlebot