Adding validatable to bus types. by n3wscott · Pull Request #321 · knative/eventing

n3wscott · 2018-08-03T22:30:47Z

This will be to support the webhook change.

google-prow-robot · 2018-08-03T22:30:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: n3wscott
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

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

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

n3wscott · 2018-08-03T22:36:06Z

Grant for LGTM:
/assign @grantr

Evan for Approval:
/assign @evankanderson

imjasonh · 2018-08-03T22:47:20Z

+)
+
+const (
+	longName = "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890"


You can use strings.Repeat to avoid having to type this out yourself, and so its self documenting as to its length.

GREAT TIP!!

scothis

/lgtm

scothis · 2018-08-06T15:58:42Z

+}
+
+func (current *Bus) CheckImmutableFields(og apis.Immutable) *apis.FieldError {
+	// TODO(n3wscott): Anything to check?


I'm not aware of any

scothis · 2018-08-06T15:58:53Z

+}
+
+func (current *ClusterBus) CheckImmutableFields(og apis.Immutable) *apis.FieldError {
+	// TODO(n3wscott): Anything to check?


I'm not aware of any

scothis · 2018-08-06T17:04:59Z

/lgtm

n3wscott · 2018-08-07T15:52:18Z

bad merge of lock file... will fix when at real computer

grantr · 2018-08-07T18:15:39Z

+
+package v1alpha1
+
+func (b *Bus) SetDefaults() {


It's fine for these no-op methods to exist but I'd like to see a comment justifying their existence, otherwise someone will remove them unknowingly. Do we expect defaults to be added in the near future? Is this a demonstration of what's possible? A desire for consistency across all resource types? A need for all objects to implement the same interface?

[n3wscott]: Note about #321 and other related Validatable stub PRs: This is staging work, the plan is another pass to bring up the test coverage, then remove unused after each type is stubbed.

Makes sense. Can you add those words as a comment in this file? Just in case this work ends up taking longer than expected. 😸

grantr · 2018-08-07T18:17:05Z

+	return nil
+}
+
+func (current *Bus) CheckImmutableFields(og apis.Immutable) *apis.FieldError {


Same as defaulters above. I'd like to see a comment justifying this method's no-op existence.

scothis · 2018-08-07T19:00:25Z

/lgtm

n3wscott · 2018-08-08T14:26:09Z

[n3wscott]: Note about #321 and other related Validatable stub PRs: This is staging work, the plan is another pass to bring up the test coverage, then remove unused after each type is stubbed.

evankanderson

/approve

evankanderson · 2018-08-08T21:50:05Z

 	BacksChannel(channel *Channel) bool
 	GetSpec() *BusSpec
+	apis.Validatable
+	apis.Immutable


Should this be apis.Defaultable as well (since you have an implementation)?

Alternately, since there appear to be no defaults, you might be able to get rid of the _defaults.go files.

evankanderson · 2018-08-08T21:53:54Z

+				},
+			},
+		},
+		want: nil,


You don't need to specify want here.

evankanderson · 2018-08-08T21:54:52Z

+				Subscription: &[]Parameter{
+					{
+						Name:        "foo",
+						Description: "bar",


Do you want to check that empty description is valid
?

evankanderson · 2018-08-08T21:55:32Z

+			},
+		},
+		want: &apis.FieldError{
+			Message: fmt.Sprintf("invalid key name %q", longName),


It would be nice if you could supply a "matcher" object here.

evankanderson · 2018-08-08T21:57:49Z

+			},
+			Details: "must be no more than 253 characters",
+		},
+	}, {


Do you want to test any of:

Multiple parameters to the same Bus.

Two parameters with the same Name in the same list.

Multiple errors in the same object.

good thought, I added a todo to add those, this was just moving what tests existed.

evankanderson · 2018-08-08T21:58:27Z

+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := test.bs.Validate()


It would be nice if you could actually put these specs into both a Bus and a ClusterBus for testing (to ensure that both are hooked up correctly).

It looks like the webhook tests actually do this right now, so I'm less concerned about this.

evankanderson · 2018-08-08T21:59:53Z

+		return err
+	}
+	if err := new.CheckImmutableFields(old); err != nil {
+		return err


It looks like we never call .SetDefaults() on new. Should we do that first, if it's going to be Defaultable? (Again, I question the value right now.)

knative-prow-robot · 2018-08-08T22:00:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, 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~~ [evankanderson]

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

knative-metrics-robot · 2018-08-08T22:14:57Z

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	Delta
pkg/apis/channels/v1alpha1/bus_defaults.go	Do not exist	0.0%
pkg/apis/channels/v1alpha1/bus_validation.go	Do not exist	87.5%
pkg/apis/channels/v1alpha1/clusterbus_defaults.go	Do not exist	0.0%
pkg/apis/channels/v1alpha1/bus_types.go	Do not exist	0.0%
pkg/apis/channels/v1alpha1/clusterbus_validation.go	Do not exist	0.0%
pkg/webhook/bus.go	77.4%	66.7%	-10.8

grantr · 2018-08-08T22:17:27Z

/lgtm

Thanks @n3wscott!

n3wscott · 2018-08-08T22:28:19Z

/test pull-knative-eventing-integration-tests

/ISTIO

knative-prow-robot requested review from mattmoor and vaikas August 3, 2018 22:30

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 3, 2018

knative-prow-robot assigned evankanderson and grantr Aug 3, 2018

imjasonh reviewed Aug 3, 2018

View reviewed changes

scothis reviewed Aug 6, 2018

View reviewed changes

knative-prow-robot assigned scothis Aug 6, 2018

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Aug 6, 2018

Scott Nichols added 4 commits August 7, 2018 11:05

Update deps.

ddae22e

Add bus validation and use it in the current webhook.

2d9525f

Use strings.Repeat, ftw.

852171a

Unit tests need to pass.

439fcc7

n3wscott force-pushed the bus_validate branch from 223f8e2 to 439fcc7 Compare August 7, 2018 18:06

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 7, 2018

grantr reviewed Aug 7, 2018

View reviewed changes

Updated dep to 0.5.0.

ecbd169

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

missed defaultable on bus interface.

5e8cc30

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

evankanderson reviewed Aug 8, 2018

View reviewed changes

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

Adding todo and context comments.

55e7d52

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

knative-prow-robot merged commit 34ab480 into knative:master Aug 8, 2018

grantr mentioned this pull request Aug 21, 2018

Add grantr to list of approvers #387

Merged

Conversation

n3wscott commented Aug 3, 2018

Uh oh!

google-prow-robot commented Aug 3, 2018

Uh oh!

n3wscott commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scothis left a comment

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!

scothis commented Aug 6, 2018

Uh oh!

n3wscott commented Aug 7, 2018

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!

Choose a reason for hiding this comment

Uh oh!

scothis commented Aug 7, 2018

Uh oh!

n3wscott commented Aug 8, 2018

Uh oh!

evankanderson left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Aug 8, 2018

Uh oh!

knative-metrics-robot commented Aug 8, 2018

Uh oh!

grantr commented Aug 8, 2018

Uh oh!

n3wscott commented Aug 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants