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

Event Registry - Sources Part 1 #300

Merged
merged 69 commits into from
May 3, 2019

Conversation

nachocano
Copy link
Contributor

@nachocano nachocano commented Mar 27, 2019

First part of sources for the Registry feature request: knative/eventing#929

Important

  • This still doesn't address the changes of CloudEvents source and subject attributes. There is still some debate of what should be put in each. This PR just sets the framework necessary to easily set those attributes in a subsequent PR.
  • Changes to Sources CRD to include description and schema attributes are not there yet.

Proposed Changes

  • Creating EventTypes in the following source controllers, from which we know (at least some of) the events they can produce:
    • GitHub
    • AWS SQS
    • GCPPubSub
    • Kafka
  • Only creating the EventTypes for Broker sinks.
  • Created a common EventType "reconciler" helper to avoid duplicating code across the source controllers.
  • Adding dependency of the eventing repo (and updating pkg)

Release Note

On installing a source CO (GitHub, AWS SQS, GCPPubSub, or Kafka), its EventTypes will be added to the Registry.

@knative-prow-robot knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 27, 2019
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 27, 2019
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

Still working through, but here are some more comments!

pkg/reconciler/eventtype/eventtype.go Outdated Show resolved Hide resolved
contrib/kafka/pkg/reconciler/kafkasource.go Outdated Show resolved Hide resolved
pkg/reconciler/eventtype/eventtype.go Outdated Show resolved Hide resolved
pkg/reconciler/eventtype/eventtype.go Outdated Show resolved Hide resolved
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

Got through everything!

pkg/reconciler/eventtype.go Outdated Show resolved Hide resolved
pkg/reconciler/eventtype.go Outdated Show resolved Hide resolved
pkg/reconciler/eventtype.go Outdated Show resolved Hide resolved
pkg/reconciler/eventtype.go Outdated Show resolved Hide resolved
@grantr grantr mentioned this pull request May 1, 2019
@nachocano
Copy link
Contributor Author

Got through everything!

Thanks a lot @grantr . Your review was very helpful! Will let you know when is ready for a second pass

@nachocano
Copy link
Contributor Author

nachocano commented May 1, 2019

Tested gcppubsub, kafka, and github... Seems to be working fine. Cannot easily test aws sqs.
Still need to update source and subject values (mainly in github adapter), will be done in the following PR.

NAME                                          TYPE                                     SOURCE                                                          SCHEMA   BROKER    DESCRIPTION   READY   REASON
dev.knative.kafka.event-cjvcr                 dev.knative.kafka.event                  topic2                                                                   default                 True    
dev.knative.kafka.event-tdt48                 dev.knative.kafka.event                  knative-demo-topic                                                       default                 True    
dev.knative.source.github.pullrequest-pkb8c   dev.knative.source.github.pull_request   nachocano/eventing                                                       default                 True    
dev.knative.source.github.push-vmbl2          dev.knative.source.github.push           nachocano/eventing                                                       default                 True    
google.pubsub.topic.publish-hrxhh             google.pubsub.topic.publish              //pubsub.googleapis.com/knative-project-228222/topics/testing            default                 True    

@nachocano
Copy link
Contributor Author

@grantr @vaikas-google @n3wscott
this is ready for a second pass. There are a couple of separate follow up issues that I can take care later on, but the basic functionality herein seems to be working (see kubectl get eventtypes output above). The main one is a refactoring of the eventtype reconciler proposed by Grant, which makes a lot of sense. I guess I didn't get it right away.

The next PR should contain the changes to source and subject.

@vaikas
Copy link
Contributor

vaikas commented May 2, 2019

/approve

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2019
@nachocano
Copy link
Contributor Author

nachocano commented May 2, 2019

Made a small refactor to make it easier for the bigger one later on. Added the eventtype package again, inside reconciler, with its own resources dir. The guys were right :)

@nachocano
Copy link
Contributor Author

@grantr @vaikas-google @n3wscott friendly ping to lgtm (or keep on adding comments) so that I can continue with the follow ups.

@vaikas
Copy link
Contributor

vaikas commented May 2, 2019

I already approved, sorry. should've said explicitly, letting @grantr @n3wscott take a look since they had some suggestiongs.

Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

Just the one change left from me (remove Raw) then 👍

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, nachocano, 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 [grantr,vaikas-google]

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

@nachocano
Copy link
Contributor Author

Just the one change left from me (remove Raw) then

Done!

@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
contrib/gcppubsub/pkg/apis/sources/v1alpha1/gcp_pubsub_types.go 100.0% 93.8% -6.2
contrib/gcppubsub/pkg/reconciler/gcppubsubsource.go 88.7% 90.0% 1.3
contrib/kafka/pkg/reconciler/kafkasource.go 86.0% 89.2% 3.2
pkg/adapter/github/adapter.go 97.9% 97.9% 0.0
pkg/apis/sources/v1alpha1/githubsource_types.go 100.0% 92.9% -7.1

@grantr
Copy link
Contributor

grantr commented May 3, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2019
@knative-prow-robot knative-prow-robot merged commit 7e31c30 into knative:master May 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

10 participants