Convert the Revision controller to use functional options to reduce boilerplate. by mattmoor · Pull Request #2447 · knative/serving

mattmoor · 2018-11-09T06:35:05Z

This explores the use of functional options to cut back on the amount of boilerplate (especially for conditions) to write table tests.

I think this is probably worth pursuing if only because it makes the tests themselves easier to maintain by going through more of our helper routines vs. constructing full condition lists by hand.

WIP building on prior changes: #2446

knative-prow-robot

@mattmoor: 0 warnings.

Details

In response to this:

This explores the use of functional options to cut back on the amount of boilerplate (especially for conditions) to write table tests.

I think this is probably worth pursuing if only because it makes the tests themselves easier to maintain by going through more of our helper routines vs. constructing full condition lists by hand.

WIP building on prior changes: #2446

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.

mattmoor · 2018-11-10T05:54:09Z

/test pull-knative-serving-integration-tests

dprotaso

I'm generally a fan of this style of initialization. Overall we might look at being consistent with the prefix of the functions.

A quick scan and I see the following

With*
Set*
Mark*
Add*
No*
Clear*
All*
Conditions*

I'd imagine choosing a single prefix would reduce the cognitive load of figuring out what options exist for a given type. Secondly, IDEs with autocomplete just typing the prefix would ideally show me the set of options I could pass to the construction of the fixture.

dprotaso · 2018-11-14T00:41:37Z

@@ -46,11 +46,11 @@ func getIsServiceReady(e *corev1.Endpoints) bool {
 }

 func getRevisionLastTransitionTime(r *v1alpha1.Revision) time.Time {


Separate PR?

dprotaso · 2018-11-14T00:43:21Z

 		return r.CreationTimestamp.Time
 	}
-	return r.Status.Conditions[condCount-1].LastTransitionTime.Inner.Time
+	return ready.LastTransitionTime.Inner.Time


Makes me think we should have a function for these transition/created times

ie.

func (c Condition) LastTransitionTime() time.Time { c.LastTransitionTime.Inner.Time }

dprotaso · 2018-11-14T01:00:27Z

-	cfg, err := resources.MakeConfiguration(svc)
+type ConfigOption func(*v1alpha1.Configuration)
+
+func config(name, namespace string, so ServiceOption, co ...ConfigOption) *v1alpha1.Configuration {


When seeing config("config", "namespace", WithPin) I couldn't infer how WithPin was manipulating the config - since I thought it modified a service.

If you only allow ConfigOption to be used I could see an alternative

config("name", "namespace", WithPinnedSpec("service", "namespace"))

where WithPinnedSpec may be defined like

func WithPinnedSpec(serviceName, serviceNamespace, so ....ServiceOption) *v1alpha1.Configuration

Maybe the namespace could be omitted

This starts to factor out functional options for setting up our table tests, which reduces the verbosity a fair amount. I've broken off Configuration from a broader set of changes in knative#2447 to make things a bit more manageable to review.

This starts to factor out functional options for setting up our table tests, which reduces the verbosity a fair amount. I've broken off Service from a broader set of changes in knative#2447 to make things a bit more manageable to review.

mattmoor · 2018-11-15T04:21:00Z

/lint

knative-prow-robot

@mattmoor: 3 warnings.

Details

In response to this:

/lint

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.

knative-prow-robot · 2018-11-15T04:21:10Z

+	}
+}
+
+// WithFailedCondif reflects the Configuration's failure in the Service


Golint comments: comment on exported function WithFailedConfig should be of the form "WithFailedConfig ...". More info.

knative-prow-robot · 2018-11-15T04:21:10Z

+	r.Status.LogURL = "http://logger.io/test-uid"
+}
+
+// TODO(mattmoor): Ideally this could be a more generic Option and use meta.Accessor,


Golint comments: comment on exported function WithCreationTimestamp should be of the form "WithCreationTimestamp ...". More info.

knative-prow-robot · 2018-11-15T04:21:11Z

+	})
+}
+
+// WithSuccessfulBuild propagates the status of a successful Build to the Revision's status.


Golint comments: comment on exported function WithSucessfulBuild should be of the form "WithSucessfulBuild ...". More info.

This starts to factor out functional options for setting up our table tests, which reduces the verbosity a fair amount. I've broken off Service from a broader set of changes in knative#2447 to make things a bit more manageable to review.

mattmoor · 2018-11-15T04:24:12Z

/lint

knative-prow-robot

@mattmoor: 1 unresolved warning and 0 new warning.

Details

In response to this:

/lint

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.

mattmoor · 2018-11-15T04:31:24Z

/test pull-knative-serving-unit-tests

This starts to factor out functional options for setting up our table tests, which reduces the verbosity a fair amount. I've broken off Configuration from a broader set of changes in knative#2447 to make things a bit more manageable to review.

This starts to factor out functional options for setting up our table tests, which reduces the verbosity a fair amount. I've broken off Configuration from a broader set of changes in #2447 to make things a bit more manageable to review.

This starts to factor out functional options for setting up our table tests, which reduces the verbosity a fair amount. I've broken off Service from a broader set of changes in knative#2447 to make things a bit more manageable to review.

jonjohnsonjr · 2018-11-16T17:21:21Z

+			Object: svc("all-ready", "foo", WithRunLatestRollout,
+				// When the Route and Configuration come back with ready
+				// our initial conditions transition to Ready=True.
+				// TODO(mattmoor): Add Latest{Created,REady}


Nit: Ready 😄

…tive#2446) An initial cleanup while looking into: knative#2442

jonjohnsonjr · 2018-11-16T18:12:38Z

/lgtm
/approve

knative-prow-robot · 2018-11-16T18:12:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonjohnsonjr, mattmoor

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:

~~pkg/reconciler/OWNERS~~ [mattmoor]

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 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2018

knative-prow-robot requested review from dprotaso, mdemirhan and tcnghia November 9, 2018 06:35

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 Nov 9, 2018

knative-prow-robot reviewed Nov 9, 2018

View reviewed changes

mattmoor force-pushed the simple-conditions branch 6 times, most recently from 700fa84 to 7e7359c Compare November 10, 2018 04:33

dprotaso reviewed Nov 14, 2018

View reviewed changes

mattmoor force-pushed the simple-conditions branch 3 times, most recently from f981591 to 8d543d7 Compare November 14, 2018 17:38

mattmoor mentioned this pull request Nov 14, 2018

Refactor the Configuration following #2447 #2493

Merged

mattmoor force-pushed the simple-conditions branch from 8d543d7 to 8b12233 Compare November 15, 2018 01:47

mattmoor mentioned this pull request Nov 15, 2018

Refactor the Service following #2447 #2500

Closed

mattmoor force-pushed the simple-conditions branch from 8b12233 to 3db11bd Compare November 15, 2018 03:59

mattmoor force-pushed the simple-conditions branch 2 times, most recently from 6654928 to 23f4fe5 Compare November 15, 2018 04:20

knative-prow-robot reviewed Nov 15, 2018

View reviewed changes

mattmoor force-pushed the simple-conditions branch 2 times, most recently from dbee406 to c43df9d Compare November 15, 2018 04:24

knative-prow-robot reviewed Nov 15, 2018

View reviewed changes

Refactor the Service following knative#2447

63ec2bf

This starts to factor out functional options for setting up our table tests, which reduces the verbosity a fair amount. I've broken off Service from a broader set of changes in knative#2447 to make things a bit more manageable to review.

mattmoor force-pushed the simple-conditions branch from c43df9d to c63ab41 Compare November 16, 2018 15:52

mattmoor changed the title ~~[WIP] PoC Use functional options to reduce boilerplace~~ Convert the Revision controller to use functional options to reduce boilerplate. Nov 16, 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 Nov 16, 2018

mattmoor assigned dprotaso and jonjohnsonjr Nov 16, 2018

jonjohnsonjr reviewed Nov 16, 2018

View reviewed changes

Simplify some of the object construction in Revision controller. (kna…

02acf77

…tive#2446) An initial cleanup while looking into: knative#2442

mattmoor force-pushed the simple-conditions branch from c63ab41 to 02acf77 Compare November 16, 2018 17:31

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

knative-prow-robot merged commit 7c4f3c5 into knative:master Nov 16, 2018

mattmoor deleted the simple-conditions branch November 16, 2018 19:08

dprotaso mentioned this pull request Jun 19, 2019

Add @dprotaso as a serving approver #4426

Merged

		@@ -46,11 +46,11 @@ func getIsServiceReady(e *corev1.Endpoints) bool {
		}

		func getRevisionLastTransitionTime(r *v1alpha1.Revision) time.Time {

Conversation

mattmoor commented Nov 9, 2018

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor commented Nov 10, 2018

Uh oh!

dprotaso 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!

mattmoor commented Nov 15, 2018

Uh oh!

knative-prow-robot 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!

mattmoor commented Nov 15, 2018

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor commented Nov 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonjohnsonjr commented Nov 16, 2018

Uh oh!

knative-prow-robot commented Nov 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants