Refactor the Configuration following #2447 by mattmoor · Pull Request #2493 · knative/serving

mattmoor · 2018-11-14T19:36:16Z

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.

knative-prow-robot · 2018-11-14T19:36:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@mattmoor: 25 warnings.

Details

In response to this:

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.

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-14T19:36:28Z

+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+)
+
+type BuildOption func(*unstructured.Unstructured)


Golint comments: exported type BuildOption should have comment or be unexported. More info.

knative-prow-robot · 2018-11-14T19:36:28Z

+	}
+}
+
+func MarkLatestCreatedFailed(msg string) ConfigOption {


Golint comments: exported function MarkLatestCreatedFailed should have comment or be unexported. More info.

knative-prow-robot · 2018-11-14T19:36:28Z

+	cfg.Status.SetLatestCreatedRevisionName(confignames.Revision(cfg))
+}
+
+func WithLatestReady(cfg *v1alpha1.Configuration) {


Golint comments: exported function WithLatestReady should have comment or be unexported. More info.

knative-prow-robot · 2018-11-14T19:36:29Z

+	}
+}
+
+func WithConfigConcurrencyModel(ss v1alpha1.RevisionRequestConcurrencyModelType) ConfigOption {


Golint comments: exported function WithConfigConcurrencyModel should have comment or be unexported. More info.

knative-prow-robot · 2018-11-14T19:36:29Z

+	}
+}
+
+func WithGeneration(gen int64) ConfigOption {


Golint comments: exported function WithGeneration should have comment or be unexported. More info.

knative-prow-robot · 2018-11-14T19:36:30Z

+	}
+}
+
+func MarkActive(r *v1alpha1.Revision) {


Golint comments: exported function MarkActive should have comment or be unexported. More info.

knative-prow-robot · 2018-11-14T19:36:31Z

+	r.Status.MarkActive()
+}
+
+func MarkContainerMissing(rev *v1alpha1.Revision) {


Golint comments: exported function MarkContainerMissing should have comment or be unexported. More info.

knative-prow-robot · 2018-11-14T19:36:31Z

+
+type K8sServiceOption func(*corev1.Service)
+
+type EndpointsOption func(*corev1.Endpoints)


Golint comments: exported type EndpointsOption should have comment or be unexported. More info.

knative-prow-robot · 2018-11-14T19:36:31Z

+	}
+}
+
+func WithRevConcurrencyModel(ss v1alpha1.RevisionRequestConcurrencyModelType) RevisionOption {


Golint comments: exported function WithRevConcurrencyModel should have comment or be unexported. More info.

knative-prow-robot · 2018-11-14T19:36:31Z

+
+type RouteOption func(*v1alpha1.Route)
+
+type ConfigOption func(*v1alpha1.Configuration)


Golint comments: exported type ConfigOption should have comment or be unexported. More info.

dgerd

Much easier to read. Nice work!

dgerd · 2018-11-14T20:37:54Z

-				}), "Bogus"),
+			Object: cfg("validation-failure", "foo", 1234, WithConfigConcurrencyModel("Bogus"),
+				// Expect Revision creation to fail with the following error.
+				MarkRevisionCreationFailed(`invalid value "Bogus": spec.concurrencyModel`)),


This looks to be using cfg.Status.MarkRevisionCreationFailed(msg). I am not sure if this method is tested elsewhere, but the previous logic had some coverage of the method validating that the message is transformed and all fields are set.

My thinking is that this is a gap in configuration_types_test.go and not a gap here.

Opened: #2499

dgerd · 2018-11-14T20:43:58Z

+			Object: cfg("need-rev-and-build", "foo", 99998, WithBuild,
+				// The following properties are set when we first reconcile a Configuration
+				// that stamps out a Revision with an existing Build.
+				WithLatestCreated, WithObservedGen),


Same as the above comment on WithLatestCreated.

serving/pkg/apis/serving/v1alpha1/configuration_types_test.go

Line 197 in d8283a8

r.Status.SetLatestCreatedRevisionName("foo")

mattmoor · 2018-11-15T01:31:09Z

/lint

knative-prow-robot

@mattmoor: 1 unresolved warnings and 1 new 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-15T01:31:25Z

+	}
+}
+
+// WithObservedGeneration sets the observed generation of the Configuration.


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

dgerd · 2018-11-15T17:16:56Z

+	}
+}
+
+// TODO(mattmoor): Ideally this could be a more generic Option and use meta.Accessor,


I don't think this TODO is happy with the linter still...

dgerd · 2018-11-15T17:17:33Z

/lint

knative-prow-robot

@dgerd: 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.

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.

dgerd · 2018-11-16T01:39:06Z

/lgtm

mattmoor · 2018-11-16T01:45:12Z

/test pull-knative-serving-integration-tests

mattmoor assigned dprotaso, jonjohnsonjr and dgerd Nov 14, 2018

knative-prow-robot requested review from dprotaso and tcnghia November 14, 2018 19:36

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 14, 2018

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

knative-prow-robot reviewed Nov 14, 2018

View reviewed changes

dgerd reviewed Nov 14, 2018

View reviewed changes

mattmoor force-pushed the refactor-config-tests branch from 669a6de to 36c7083 Compare November 15, 2018 01:30

knative-prow-robot reviewed Nov 15, 2018

View reviewed changes

mattmoor force-pushed the refactor-config-tests branch 2 times, most recently from 9e75bcc to 5930e46 Compare November 15, 2018 01:38

This was referenced Nov 15, 2018

Unit test coverage for MarkRevisionCreationFailed #2499

Closed

Refactor the Service following #2447 #2500

Closed

dgerd reviewed Nov 15, 2018

View reviewed changes

knative-prow-robot reviewed Nov 15, 2018

View reviewed changes

mattmoor force-pushed the refactor-config-tests branch from 5930e46 to 0e33e0b Compare November 16, 2018 01:21

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

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

mattmoor deleted the refactor-config-tests branch November 16, 2018 02:20

dgerd mentioned this pull request Feb 14, 2019

Add dgerd to list of serving-api-approvers #3228

Merged


		type K8sServiceOption func(*corev1.Service)

		type EndpointsOption func(*corev1.Endpoints)


		type RouteOption func(*v1alpha1.Route)

		type ConfigOption func(*v1alpha1.Configuration)

Conversation

mattmoor commented Nov 14, 2018

Uh oh!

knative-prow-robot commented Nov 14, 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!

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!

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

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!

dgerd commented Nov 15, 2018

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

dgerd commented Nov 16, 2018

Uh oh!

mattmoor commented Nov 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants