Remove `SetDefaults` from Service() function #6202

taragu · 2019-12-11T19:17:41Z

/lint

Proposed Changes

Remove SetDefaults from default service creation so that ServiceOption's can override the defaults. Also update unit tests to set defaults for services.

Release Note

NONE

/cc @nimakaviani

knative-prow-robot

@taragu: 0 warnings.

In response to this:

/lint

Proposed Changes

Moving SetDefaults before ServiceOption's are applied so that ServiceOption's can override the defaults

Release Note
NONE
/cc @nimakaviani

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.

vagababov · 2019-12-11T19:28:49Z

/approve
I'll leave lgtm to Dan
/assign @dgerd

dgerd · 2019-12-11T22:56:04Z

I'm a bit confused why this is needed. Defaults should only be set on unspecified fields (nil, "", or 0 values). If you are overriding a field with ServiceOptions then the defaulter should skip over this.

What is this trying to unblock?

nimakaviani · 2019-12-11T23:15:14Z

this is particularly to allow for ServiceOptions to remove or modify defaults, if needed.

Note that in tests, this defaulting is done when creating the service object and before it is sent to the serving api, so this gives more flexibility to the tests to decide on the shape of the object that hits the api endpoints.

Also assuming that the code implements Dave Cheney's functional options pattern, semantically it makes more sense for the options to be the last to operate on the service object

dgerd · 2019-12-12T00:23:06Z

this is particularly to allow for ServiceOptions to remove or modify defaults, if needed.

I still am not following... If I write a service option that sets revisionTimeoutSeconds: 301 when the defaulter runs it does not touch that value since it is set. Shouldn't this be the same as defaulting revisionTimeoutSeconds: 300 and then overriding the value to 301.

I don't see the point in changing this unless I am missing something.

dgerd · 2019-12-12T00:28:40Z

And to clarify the reason why I don't think we should change this is that we use this function in our unit tests where our webhook is not called to set defaults. The pattern here of defaulting after the user provides the full Service they desire is what will happen when actually running Knative. Moving defaulting first could mask problems in our defaulter where we accidentally modify values set by the user.

nimakaviani · 2019-12-12T00:38:51Z

what if in a test you want to remove a default value and test for the impact?

we use this function in our unit tests where our webhook is not called to set defaults. The pattern here of defaulting after the user provides the full Service they desire is what will happen when actually running Knative. Moving defaulting first could mask problems in our defaulter where we accidentally modify values set by the user.

In the above case, I would consider it a bug in setting up the unit tests and the issue is isolated to the test that uses the ServiceOption in an improper way.

The bigger problem is that we use the same function for creating a service object in integration tests too. This means every service object will have the defaults set before making it to the test cluster, which will mask problems with how defaulting would work in webhooks. I think that is more of an issue in fact.

dgerd · 2019-12-12T01:00:09Z

The bigger problem is that we use the same function for creating a service object in integration tests too. This means every service object will have the defaults set before making it to the test cluster, which will mask problems with how defaulting would work in webhooks. I think that is more of an issue in fact.

I agree that does sound problematic and we should fix that.

knative-prow-robot · 2019-12-12T19:10:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: taragu, vagababov

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:

~~pkg/reconciler/OWNERS~~ [vagababov]
~~pkg/testing/OWNERS~~ [vagababov]

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

nimakaviani

@taragu you probably want the ServiceOption that sets the defaults to be the first in unit tests.

To @dgerd 's point, since the behavior is supposed to imitate the webhook, defaulting should apply to the service before the other opts.

knative-test-reporter-robot · 2019-12-13T14:45:51Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-unit-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

pkg/reconciler/nscert.TestDomainConfigDefaultDomain

taragu · 2019-12-13T15:13:49Z

/retest

taragu · 2019-12-13T16:06:59Z

/retest

nimakaviani · 2019-12-13T18:46:09Z

pkg/reconciler/service/service_test.go

-func config(name, namespace string, so ServiceOption, co ...ConfigOption) *v1alpha1.Configuration {
-	s := Service(name, namespace, so)
+func config(name, namespace string, sos []ServiceOption, co ...ConfigOption) *v1alpha1.Configuration {
+	s := Service(name, namespace, sos...)
 	s.SetDefaults(v1.WithUpgradeViaDefaulting(context.Background()))


looks like we apply defaulting to the service in config and route. I wonder if it is necessary to pass the defaulting ServiceOption through. or if we do, we should probably call it with v1.WithUpgradeViaDefaulting(context.Background() and then remove the line above?

nimakaviani · 2019-12-13T18:46:19Z

pkg/reconciler/service/service_test.go

-func route(name, namespace string, so ServiceOption, ro ...RouteOption) *v1alpha1.Route {
-	s := Service(name, namespace, so)
+func route(name, namespace string, sos []ServiceOption, ro ...RouteOption) *v1alpha1.Route {
+	s := Service(name, namespace, sos...)


same as for config

dgerd · 2019-12-16T19:27:26Z

Thanks for the updates. Can you change the commit and PR description? Otherwise this looks good to me.

Given its ubiquity in unit tests I could see DefaultedService() as an appealing helper function so I don't forget to add WithServiceDefaults at the end of my argument list every time. But I will leave that up to your discretion.

taragu · 2019-12-16T20:32:09Z

Thanks @dgerd ! I've updated the description and added the default service helper method. Please review.

dgerd · 2019-12-16T20:41:29Z

pkg/testing/v1alpha1/service.go

+	s := &v1alpha1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+	}
+	for _, opt := range so {
+		opt(s)
+	}
 	s.SetDefaults(context.Background())
 	return s


Why not reuse the existing Service function by calling it?

Service(name, namespace, so, WithServiceDefaults)

dgerd · 2019-12-16T20:44:00Z

For commit names you will want to keep the description shorter and then add details in a paragraph below. This helps it render correctly on Github and keeps it readable on command line tools.

For example,

Remove `SetDefaults` from Service() function

This change removes `SetDefaults` so that `ServiceOption`'s can override the defaults. 
Also update unit tests to set defaults for services.

This change removes `SetDefaults` so that `ServiceOption`'s can override the defaults. Also update unit tests to set defaults for services.

taragu · 2019-12-16T21:32:52Z

@dgerd thanks for the suggestions! I've updated the PR. Would you please review again?

dgerd

One last comment. I thought I had already sent this.

dgerd · 2019-12-16T20:49:46Z

pkg/testing/v1alpha1/service.go

+
+// DefaultService creates a service with ServiceOptions and with default values set
+func DefaultService(name, namespace string, so ...ServiceOption) *v1alpha1.Service {
+	s := Service(name, namespace, so...)


You already have a function for defaults. You can simplify this further :)

Suggested change

s := Service(name, namespace, so...)

s := Service(name, namespace, append(so,WithServiceDefaults)...)

dgerd · 2019-12-16T21:45:50Z

pkg/testing/v1alpha1/service.go

 	return s
 }

+// DefaultService creates a service with ServiceOptions and with default values set
+func DefaultService(name, namespace string, so ...ServiceOption) *v1alpha1.Service {
+	s := Service(name, namespace, append(so,WithServiceDefaults)...)


Now that you have combined it all into one line you don't need a variable or separate return statement.

Suggested change

s := Service(name, namespace, append(so,WithServiceDefaults)...)

return Service(name, namespace, append(so,WithServiceDefaults)...)

That is actually logically wrong

dgerd · 2019-12-16T22:15:34Z

/lgtm
/hold
for victor as I am not sure the logic is wrong.

vagababov · 2019-12-16T22:19:26Z

/hold cancel
As discussed, I am presuming worst case, which should not happen [case being that Defaults is applied after all so and I've seen this fail before, but probably here we're safe].

knative-prow-robot requested a review from nimakaviani December 11, 2019 19:17

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Dec 11, 2019

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 11, 2019

knative-prow-robot reviewed Dec 11, 2019

View reviewed changes

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Dec 11, 2019

knative-prow-robot assigned dgerd Dec 11, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2019

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 12, 2019

knative-prow-robot added the area/API API objects and controllers label Dec 12, 2019

nimakaviani reviewed Dec 12, 2019

View reviewed changes

taragu force-pushed the e2e-set-defaults branch from 57c0acc to 2c4bb0b Compare December 13, 2019 14:24

taragu force-pushed the e2e-set-defaults branch from 2c4bb0b to b45d0c3 Compare December 13, 2019 17:57

nimakaviani reviewed Dec 13, 2019

View reviewed changes

taragu force-pushed the e2e-set-defaults branch 2 times, most recently from 3ea4b86 to 6011e70 Compare December 13, 2019 19:43

taragu force-pushed the e2e-set-defaults branch from 6011e70 to f8b7be4 Compare December 16, 2019 20:02

dgerd reviewed Dec 16, 2019

View reviewed changes

Remove SetDefaults from Service() function

cf5efbd

This change removes `SetDefaults` so that `ServiceOption`'s can override the defaults. Also update unit tests to set defaults for services.

taragu force-pushed the e2e-set-defaults branch from e26fa25 to cf5efbd Compare December 16, 2019 20:45

taragu changed the title ~~Apply ServiceOptions after setting defaults~~ Remove SetDefaults from Service() function Dec 16, 2019

dgerd reviewed Dec 16, 2019

View reviewed changes

review

9767d98

dgerd reviewed Dec 16, 2019

View reviewed changes

review

be10310

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Dec 16, 2019

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2019

knative-prow-robot merged commit 310886a into knative:master Dec 16, 2019

taragu deleted the e2e-set-defaults branch January 2, 2020 12:28

taragu restored the e2e-set-defaults branch February 18, 2020 15:07

taragu deleted the e2e-set-defaults branch February 18, 2020 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `SetDefaults` from Service() function #6202

Remove `SetDefaults` from Service() function #6202

taragu commented Dec 11, 2019 •

edited

Loading

knative-prow-robot left a comment

vagababov commented Dec 11, 2019

dgerd commented Dec 11, 2019

nimakaviani commented Dec 11, 2019

dgerd commented Dec 12, 2019

dgerd commented Dec 12, 2019

nimakaviani commented Dec 12, 2019

dgerd commented Dec 12, 2019

knative-prow-robot commented Dec 12, 2019

nimakaviani left a comment

knative-test-reporter-robot commented Dec 13, 2019

taragu commented Dec 13, 2019

taragu commented Dec 13, 2019

nimakaviani Dec 13, 2019

nimakaviani Dec 13, 2019

dgerd commented Dec 16, 2019

taragu commented Dec 16, 2019

dgerd Dec 16, 2019

dgerd commented Dec 16, 2019

taragu commented Dec 16, 2019

dgerd left a comment

dgerd Dec 16, 2019

dgerd Dec 16, 2019

vagababov Dec 16, 2019

dgerd commented Dec 16, 2019

vagababov commented Dec 16, 2019

	s := Service(name, namespace, so...)
	s := Service(name, namespace, append(so,WithServiceDefaults)...)

	s := Service(name, namespace, append(so,WithServiceDefaults)...)
	return Service(name, namespace, append(so,WithServiceDefaults)...)

Remove SetDefaults from Service() function #6202

Remove SetDefaults from Service() function #6202

Conversation

taragu commented Dec 11, 2019 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

vagababov commented Dec 11, 2019

dgerd commented Dec 11, 2019

nimakaviani commented Dec 11, 2019

dgerd commented Dec 12, 2019

dgerd commented Dec 12, 2019

nimakaviani commented Dec 12, 2019

dgerd commented Dec 12, 2019

knative-prow-robot commented Dec 12, 2019

nimakaviani left a comment

Choose a reason for hiding this comment

knative-test-reporter-robot commented Dec 13, 2019

taragu commented Dec 13, 2019

taragu commented Dec 13, 2019

nimakaviani Dec 13, 2019

Choose a reason for hiding this comment

nimakaviani Dec 13, 2019

Choose a reason for hiding this comment

dgerd commented Dec 16, 2019

taragu commented Dec 16, 2019

dgerd Dec 16, 2019

Choose a reason for hiding this comment

dgerd commented Dec 16, 2019

taragu commented Dec 16, 2019

dgerd left a comment

Choose a reason for hiding this comment

dgerd Dec 16, 2019

Choose a reason for hiding this comment

dgerd Dec 16, 2019

Choose a reason for hiding this comment

vagababov Dec 16, 2019

Choose a reason for hiding this comment

dgerd commented Dec 16, 2019

vagababov commented Dec 16, 2019

Remove `SetDefaults` from Service() function #6202

Remove `SetDefaults` from Service() function #6202

taragu commented Dec 11, 2019 •

edited

Loading