Adding Source type. by n3wscott · Pull Request #457 · knative/eventing

n3wscott · 2018-09-21T20:57:09Z

Fixes #445

Proposed Changes

Adding Source.eventing.knative.dev

Release Note

NONE

n3wscott · 2018-09-21T22:32:53Z

/assign @grantr

/cc @vaikas-google

adamharwayne · 2018-09-24T15:59:32Z

+	//   - APIVersion
+	//   - Name
+	// Currently Kind must be "Channel" and
+	// APIVersion must be "channels.knative.dev/v1alpha1"


eventing.knative.dev/v1alpha1

adamharwayne · 2018-09-24T16:01:30Z

+
+// Valid from only contains the following fields:
+// - Kind       == 'Channel'
+// - APIVersion == 'channels.knative.dev/v1alpha1'


eventing.knative.dev/v1alpha1

grantr

Looks reasonable, but I'm surprised by the Channel spec field. It may be that I missed the discussion in which this field was decided on.

/cc @evankanderson

grantr · 2018-09-24T23:13:22Z

+	// ObservedGeneration is the 'Generation' of the Source that
+	// was last reconciled by the controller.
+	// +optional
+	ObservedGeneration int64 `json:"observedGeneration,omitempty"`


Not really part of this PR, but I just noticed Subscription doesn't have an ObservedGeneration field. Should it?

grantr · 2018-09-24T23:22:56Z

+// Name, Kind, APIVersion
+// If any other fields are set and is not the Zero value, returns an apis.FieldError
+// with the fieldpaths for all those fields.
+func checkDisallowedObjectReferenceFields(f corev1.ObjectReference) *apis.FieldError {


How does this look with hardcoded fields instead of reflection? I think that might be easier to read, and the fields of ObjectReference are unlikely to change.

I copied this from Ville, perhaps we can do a followup to clear this up if we need to

subscribable_validation.go is a copy and rename of original subscription methods that are useful for sources

grantr

/approve
/lgtm

Thanks @n3wscott for this PR and helping resolve the confusion around interface contracts. 😁 I think there's further discussion to be had with the group about the specifics of those contracts but I'm happy to move this PR forward.

These suggested changes can go in a followup PR:

stop using reflection to validate ObjectReferences.
Move Subscribable and Channelable contract validations to Subscription controller reconcile since they'll require the object content to verify.

knative-prow-robot · 2018-09-25T18:49:39Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [grantr]

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

grantr · 2018-09-25T18:49:49Z

/hold
(@n3wscott has more to get in)

grantr · 2018-09-25T18:50:30Z

/lgtm
/hold cancel

knative-metrics-robot · 2018-09-25T18:51:58Z

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

File	Old Coverage	New Coverage
pkg/apis/eventing/v1alpha1/source_defaults.go	Do not exist	100.0%
pkg/apis/eventing/v1alpha1/source_types.go	Do not exist	100.0%
pkg/apis/eventing/v1alpha1/source_validation.go	Do not exist	100.0%
pkg/apis/eventing/v1alpha1/subscribable_channelable_validation.go	Do not exist	93.8%

vaikas · 2018-09-26T10:18:33Z

+	// +optional
+	ObservedGeneration int64 `json:"observedGeneration,omitempty"`
+
+	// Source might be Subscribable. This points to the Channelable object.


might? I think it must be Subscribable, how else would we get events from it? Is this CRD the "factory" for say, creating K8S events for consumption by the system, or is an instance one that I could create a Subscription.Spec.From to point to?

Updates to ci run

[release-v1.12 ] Trusted CA bundle CM work

Scott Nichols added 2 commits September 21, 2018 13:54

Added the shell of a source. followup with real insides.

707e396

adding generated parts.

2c52c3a

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 21, 2018

knative-prow-robot requested review from grantr and vaikas September 21, 2018 20:57

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 21, 2018

Scott Nichols added 3 commits September 21, 2018 15:24

added source spec fields and tests.

c086a4a

update the comment for source resource.

98a3129

use isSubscribableEmpty.

bccf1c8

n3wscott changed the title ~~[WIP] Adding Source type.~~ Adding Source type. Sep 21, 2018

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 21, 2018

knative-prow-robot assigned grantr Sep 21, 2018

adamharwayne reviewed Sep 24, 2018

View reviewed changes

Scott Nichols added 2 commits September 24, 2018 10:11

Update comments to use the correct group.

1fcb560

Merge branch 'master' into source_crd

6dd1d5c

grantr reviewed Sep 24, 2018

View reviewed changes

knative-prow-robot requested a review from evankanderson September 24, 2018 23:26

Scott Nichols added 5 commits September 25, 2018 10:29

one day I will spell its correctly.

2f6dd40

Merge branch 'master' into source_crd

08be132

ducks in a row.

6b94532

Clear up Channelable and Subscribable elements inside of source.

9290da4

update codegen.

0988394

grantr reviewed Sep 25, 2018

View reviewed changes

Comment thread pkg/apis/eventing/v1alpha1/source_validation.go Outdated

Comment thread pkg/apis/eventing/v1alpha1/source_types.go Outdated

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 25, 2018

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2018

use sub helper.

7266954

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2018

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 25, 2018

knative-prow-robot merged commit 1579b12 into knative:master Sep 25, 2018

vaikas reviewed Sep 26, 2018

View reviewed changes

matzew added a commit to matzew/eventing that referenced this pull request Mar 2, 2020

Merge pull request knative#457 from matzew/updates_to_ci_run

6b8444c

Updates to ci run

Leo6Leo pushed a commit to Leo6Leo/eventing that referenced this pull request Jan 17, 2024

Merge pull request knative#457 from matzew/cm_bundle_work_1.12

06c1c47

[release-v1.12 ] Trusted CA bundle CM work

Conversation

n3wscott commented Sep 21, 2018

Proposed Changes

Uh oh!

n3wscott commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

knative-prow-robot commented Sep 25, 2018

Uh oh!

grantr commented Sep 25, 2018

Uh oh!

grantr commented Sep 25, 2018

Uh oh!

knative-metrics-robot commented Sep 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants