Repository: knative/pkg
Pull request #1308 (2020-05-06 18:48:38)

Ensure that all needed imports are tracked in reconciler generator
Currently it is possible for the generator to omit these imports, although this behavior appears to depend on the generator dependency versions.

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1308#" title="Author self-approved">ian-mi</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor**
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](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mattmoor"]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[codegen/cmd/injection-gen/generators/reconciler_reconciler.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1308/pull-knative-pkg-go-coverage/1258106538971631619/artifacts/line-cov.html#file0) | Do not exist | 0.0% | 

knative-prow-robot: @ian-mi: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-go-coverage | bcb99d85726641558d9b7a22e4a323538699cf7f | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1308/pull-knative-pkg-go-coverage/1258106538971631619) | `/test pull-knative-pkg-go-coverage`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1308). [Your PR dashboard](https://gubernator.knative.dev/pr/ian-mi).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
ian-mi: /assign @n3wscott 

Patch:
diff --git a/apis/condition_set.go b/apis/condition_set.go
index 49f9d2e9b..8d02fec48 100644
--- a/apis/condition_set.go
+++ b/apis/condition_set.go
@@ -142,6 +142,11 @@ type conditionsImpl struct {
 	accessor ConditionsAccessor
 }
 
+// GetTopLevelConditionType is an accessor for the top-level happy condition.
+func (r ConditionSet) GetTopLevelConditionType() ConditionType {
+	return r.happy
+}
+
 // Manage creates a ConditionManager from an accessor object using the original
 // ConditionSet as a reference. Status must be a pointer to a struct.
 func (r ConditionSet) Manage(status ConditionsAccessor) ConditionManager {
diff --git a/apis/duck/v1/kresource_type.go b/apis/duck/v1/kresource_type.go
index b64977fcb..80558428c 100644
--- a/apis/duck/v1/kresource_type.go
+++ b/apis/duck/v1/kresource_type.go
@@ -26,7 +26,7 @@ import (
 	"knative.dev/pkg/apis"
 )
 
-// KRShaped is an interface for retrieving the duck elements of an arbitraty resource.
+// KRShaped is an interface for retrieving the duck elements of an arbitrary resource.
 type KRShaped interface {
 	metav1.ObjectMetaAccessor
 
@@ -34,7 +34,7 @@ type KRShaped interface {
 
 	GetStatus() *Status
 
-	GetTopLevelConditionType() apis.ConditionType
+	GetConditionSet() apis.ConditionSet
 }
 
 // Asserts KResource conformance with KRShaped
@@ -91,12 +91,12 @@ func (t *KResource) GetStatus() *Status {
 	return &t.Status
 }
 
-// GetTopLevelConditionType retrieves the happy condition of this resource. Implements the KRShaped interface.
-func (t *KResource) GetTopLevelConditionType() apis.ConditionType {
+// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
+func (t *KResource) GetConditionSet() apis.ConditionSet {
 	// Note: KResources are unmarshalled from existing resources. This will only work properly for resources that
 	// have already been initialized to their type.
 	if cond := t.Status.GetCondition(apis.ConditionSucceeded); cond != nil {
-		return apis.ConditionSucceeded
+		return apis.NewBatchConditionSet()
 	}
-	return apis.ConditionReady
+	return apis.NewLivingConditionSet()
 }
diff --git a/client/injection/apiextensions/reconciler/apiextensions/v1beta1/customresourcedefinition/reconciler.go b/client/injection/apiextensions/reconciler/apiextensions/v1beta1/customresourcedefinition/reconciler.go
index 5c870fc4d..5e9b40921 100644
--- a/client/injection/apiextensions/reconciler/apiextensions/v1beta1/customresourcedefinition/reconciler.go
+++ b/client/injection/apiextensions/reconciler/apiextensions/v1beta1/customresourcedefinition/reconciler.go
@@ -20,18 +20,18 @@ package customresourcedefinition
 
 import (
 	context "context"
-	"encoding/json"
-	"reflect"
+	json "encoding/json"
+	reflect "reflect"
 
 	zap "go.uber.org/zap"
 	v1 "k8s.io/api/core/v1"
 	v1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	clientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1beta1"
-	"k8s.io/apimachinery/pkg/api/equality"
+	equality "k8s.io/apimachinery/pkg/api/equality"
 	errors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
+	types "k8s.io/apimachinery/pkg/types"
 	sets "k8s.io/apimachinery/pkg/util/sets"
 	cache "k8s.io/client-go/tools/cache"
 	record "k8s.io/client-go/tools/record"
diff --git a/client/injection/kube/reconciler/core/v1/namespace/reconciler.go b/client/injection/kube/reconciler/core/v1/namespace/reconciler.go
index 2893639c3..10bbab032 100644
--- a/client/injection/kube/reconciler/core/v1/namespace/reconciler.go
+++ b/client/injection/kube/reconciler/core/v1/namespace/reconciler.go
@@ -20,15 +20,15 @@ package namespace
 
 import (
 	context "context"
-	"encoding/json"
-	"reflect"
+	json "encoding/json"
+	reflect "reflect"
 
 	zap "go.uber.org/zap"
 	v1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/equality"
+	equality "k8s.io/apimachinery/pkg/api/equality"
 	errors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
+	types "k8s.io/apimachinery/pkg/types"
 	sets "k8s.io/apimachinery/pkg/util/sets"
 	kubernetes "k8s.io/client-go/kubernetes"
 	corev1 "k8s.io/client-go/listers/core/v1"
diff --git a/codegen/cmd/injection-gen/generators/reconciler_reconciler.go b/codegen/cmd/injection-gen/generators/reconciler_reconciler.go
index d1ceb54ed..0d098ab60 100644
--- a/codegen/cmd/injection-gen/generators/reconciler_reconciler.go
+++ b/codegen/cmd/injection-gen/generators/reconciler_reconciler.go
@@ -145,6 +145,10 @@ func (g *reconcilerReconcilerGenerator) GenerateType(c *generator.Context, t *ty
 			Package: "context",
 			Name:    "Context",
 		}),
+		"reflectDeepEqual":    c.Universe.Package("reflect").Function("DeepEqual"),
+		"equalitySemantic":    c.Universe.Package("k8s.io/apimachinery/pkg/api/equality").Variable("Semantic"),
+		"jsonMarshal":         c.Universe.Package("encoding/json").Function("Marshal"),
+		"typesMergePatchType": c.Universe.Package("k8s.io/apimachinery/pkg/types").Constant("MergePatchType"),
 	}
 
 	sw.Do(reconcilerInterfaceFactory, m)
@@ -321,7 +325,7 @@ func (r *reconcilerImpl) Reconcile(ctx {{.contextContext|raw}}, key string) erro
 	}
 
 	// Synchronize the status.
-	if equality.Semantic.DeepEqual(original.Status, resource.Status) {
+	if {{.equalitySemantic|raw}}.DeepEqual(original.Status, resource.Status) {
 		// If we didn't change anything then don't call updateStatus.
 		// This is important because the copy we loaded from the injectionInformer's
 		// cache may be stale and we don't want to overwrite a prior update
@@ -368,7 +372,7 @@ func (r *reconcilerImpl) updateStatus(existing *{{.type|raw}}, desired *{{.type|
 		}
 
 		// If there's nothing to update, just return.
-		if reflect.DeepEqual(existing.Status, desired.Status) {
+		if {{.reflectDeepEqual|raw}}(existing.Status, desired.Status) {
 			return nil
 		}
 
@@ -433,7 +437,7 @@ func (r *reconcilerImpl) updateFinalizersFiltered(ctx {{.contextContext|raw}}, r
 		},
 	}
 
-	patch, err := json.Marshal(mergePatch)
+	patch, err := {{.jsonMarshal|raw}}(mergePatch)
 	if err != nil {
 		return resource, err
 	}
@@ -443,7 +447,7 @@ func (r *reconcilerImpl) updateFinalizersFiltered(ctx {{.contextContext|raw}}, r
 	{{else}}
 	patcher := r.Client.{{.group}}{{.version}}().{{.type|apiGroup}}(resource.Namespace)
 	{{end}}
-	resource, err = patcher.Patch(resource.Name, types.MergePatchType, patch)
+	resource, err = patcher.Patch(resource.Name, {{.typesMergePatchType|raw}}, patch)
 	if err != nil {
 		r.Recorder.Eventf(resource, {{.corev1EventTypeWarning|raw}}, "FinalizerUpdateFailed",
 			"Failed to update finalizers for %q: %v", resource.Name, err)
diff --git a/logging/object_encoders.go b/logging/object_encoders.go
new file mode 100644
index 000000000..b6f39fe21
--- /dev/null
+++ b/logging/object_encoders.go
@@ -0,0 +1,59 @@
+/*
+Copyright 2020 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package logging
+
+import (
+	"strings"
+
+	"go.uber.org/zap/zapcore"
+
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
+)
+
+// This file contains the specific object encoders for use in Knative
+// to optimize logging experience and performance.
+
+// StringSet returns a marshaler for the set of strings.
+// To use this in sugared logger do:
+//	logger.Infow("Revision State", zap.Object("healthy", logging.StringSet(healthySet)),
+//		zap.Object("unhealthy", logging.StringSet(unhealthySet)))
+// To use with non-sugared logger do:
+//	logger.Info("Revision State", zap.Object("healthy", logging.StringSet(healthySet)),
+//		zap.Object("unhealthy", logging.StringSet(unhealthySet)))
+func StringSet(s sets.String) zapcore.ObjectMarshalerFunc {
+	return func(enc zapcore.ObjectEncoder) error {
+		enc.AddString("keys", strings.Join(s.UnsortedList(), ","))
+		return nil
+	}
+}
+
+// NamespacedName returns a marshaler for NamespacedName.
+// To use this in sugared logger do:
+//	logger.Infow("Enqueuing", zap.Object("key", logging.NamespacedName(n)))
+// To use with non-sugared logger do:
+//	logger.Info("Enqueuing", zap.Object("key", logging.NamespacedName(n)))
+func NamespacedName(n types.NamespacedName) zapcore.ObjectMarshalerFunc {
+	return func(enc zapcore.ObjectEncoder) error {
+		if n.Namespace != "" {
+			enc.AddString("key", n.Name)
+		} else {
+			enc.AddString("key", n.Namespace+"/"+n.Name)
+		}
+		return nil
+	}
+}
diff --git a/reconciler/reconcile_common.go b/reconciler/reconcile_common.go
new file mode 100644
index 000000000..38e3bbe38
--- /dev/null
+++ b/reconciler/reconcile_common.go
@@ -0,0 +1,55 @@
+/*
+Copyright 2020 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package reconciler
+
+import (
+	"context"
+
+	duckv1 "knative.dev/pkg/apis/duck/v1"
+	"knative.dev/pkg/logging"
+)
+
+const failedGenerationBump = "NewObservedGenFailure"
+
+// PreProcessReconcile contains logic to apply before reconciliation of a resource.
+func PreProcessReconcile(ctx context.Context, resource duckv1.KRShaped) {
+	newStatus := resource.GetStatus()
+
+	if newStatus.ObservedGeneration != resource.GetObjectMeta().GetGeneration() {
+		condSet := resource.GetConditionSet()
+		manager := condSet.Manage(newStatus)
+
+		// Reset Ready/Successful to unknown. The reconciler is expected to overwrite this.
+		manager.MarkUnknown(condSet.GetTopLevelConditionType(), failedGenerationBump, "unsuccessfully observed a new generation")
+	}
+}
+
+// PostProcessReconcile contains logic to apply after reconciliation of a resource.
+func PostProcessReconcile(ctx context.Context, resource duckv1.KRShaped) {
+	logger := logging.FromContext(ctx)
+	newStatus := resource.GetStatus()
+	mgr := resource.GetConditionSet().Manage(newStatus)
+
+	// Bump observed generation to denote that we have processed this
+	// generation regardless of success or failure.
+	newStatus.ObservedGeneration = resource.GetObjectMeta().GetGeneration()
+
+	rc := mgr.GetTopLevelCondition()
+	if rc.Reason == failedGenerationBump {
+		logger.Warn("A reconciler observed a new generation without updating the resource status")
+	}
+}


Test patch:
diff --git a/apis/duck/v1/kresource_type_test.go b/apis/duck/v1/kresource_type_test.go
index 357353b14..e5c7f9371 100644
--- a/apis/duck/v1/kresource_type_test.go
+++ b/apis/duck/v1/kresource_type_test.go
@@ -22,22 +22,35 @@ import (
 	"knative.dev/pkg/apis"
 )
 
-func TestGetTopLevelCondition(t *testing.T) {
-	resource := KResource{}
-
-	condSet := apis.NewLivingConditionSet("Foo")
-	mgr := condSet.Manage(resource.GetStatus())
-	mgr.InitializeConditions()
-
-	if resource.GetTopLevelConditionType() != apis.ConditionReady {
-		t.Error("Expected Ready as happy condition for living condition set type")
+func TestGetConditionSet(t *testing.T) {
+	testCases := []struct {
+		name                      string
+		condSet                   apis.ConditionSet
+		expectedTopLevelCondition apis.ConditionType
+	}{
+		{
+			name:                      "living set",
+			condSet:                   apis.NewLivingConditionSet(),
+			expectedTopLevelCondition: apis.ConditionReady,
+		},
+		{
+			name:                      "batch set",
+			condSet:                   apis.NewBatchConditionSet(),
+			expectedTopLevelCondition: apis.ConditionSucceeded,
+		},
 	}
 
-	condSet = apis.NewBatchConditionSet("Foo")
-	mgr = condSet.Manage(resource.GetStatus())
-	mgr.InitializeConditions()
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			resource := KResource{}
+			test.condSet.Manage(resource.GetStatus()).InitializeConditions()
+
+			mgr := resource.GetConditionSet().Manage(resource.GetStatus())
 
-	if resource.GetTopLevelConditionType() != apis.ConditionSucceeded {
-		t.Error("Expected Succeeded as happy condition for living condition set type")
+			if mgr.GetTopLevelCondition().Type != test.expectedTopLevelCondition {
+				t.Errorf("wrong top-level condition got=%s want=%s",
+					mgr.GetTopLevelCondition().Type, test.expectedTopLevelCondition)
+			}
+		})
 	}
 }
diff --git a/apis/test/example/v1alpha1/fiz_types.go b/apis/test/example/v1alpha1/fiz_types.go
index 334e827a5..b87559e2e 100644
--- a/apis/test/example/v1alpha1/fiz_types.go
+++ b/apis/test/example/v1alpha1/fiz_types.go
@@ -104,7 +104,7 @@ func (f *ClusterFiz) GetStatus() *duckv1.Status {
 	return &f.Status.Status
 }
 
-// GetTopLevelConditionType retrieves the happy condition of this resource. Implements the KRShaped interface.
-func (*ClusterFiz) GetTopLevelConditionType() apis.ConditionType {
-	return apis.ConditionReady
+// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
+func (*ClusterFiz) GetConditionSet() apis.ConditionSet {
+	return apis.NewLivingConditionSet()
 }
diff --git a/apis/test/example/v1alpha1/foo_types.go b/apis/test/example/v1alpha1/foo_types.go
index 948254353..232612732 100644
--- a/apis/test/example/v1alpha1/foo_types.go
+++ b/apis/test/example/v1alpha1/foo_types.go
@@ -103,7 +103,7 @@ func (f *Foo) GetStatus() *duckv1.Status {
 	return &f.Status.Status
 }
 
-// GetTopLevelConditionType retrieves the happy condition of this resource. Implements the KRShaped interface.
-func (*Foo) GetTopLevelConditionType() apis.ConditionType {
-	return apis.ConditionSucceeded
+// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
+func (*Foo) GetConditionSet() apis.ConditionSet {
+	return apis.NewLivingConditionSet()
 }
diff --git a/apis/test/pub/v1alpha1/bar_types.go b/apis/test/pub/v1alpha1/bar_types.go
index f27d5f660..2227a6142 100644
--- a/apis/test/pub/v1alpha1/bar_types.go
+++ b/apis/test/pub/v1alpha1/bar_types.go
@@ -103,7 +103,7 @@ func (b *Bar) GetStatus() *duckv1.Status {
 	return &b.Status.Status
 }
 
-// GetTopLevelConditionType retrieves the happy condition of this resource. Implements the KRShaped interface.
-func (*Bar) GetTopLevelConditionType() apis.ConditionType {
-	return apis.ConditionReady
+// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
+func (*Bar) GetConditionSet() apis.ConditionSet {
+	return apis.NewLivingConditionSet()
 }
diff --git a/reconciler/reconcile_common_test.go b/reconciler/reconcile_common_test.go
new file mode 100644
index 000000000..b61cc0399
--- /dev/null
+++ b/reconciler/reconcile_common_test.go
@@ -0,0 +1,96 @@
+/*
+Copyright 2020 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package reconciler
+
+import (
+	"context"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"knative.dev/pkg/apis"
+	duckv1 "knative.dev/pkg/apis/duck/v1"
+)
+
+func makeResource(topLevelCond string) *duckv1.KResource {
+	fooCond := apis.Condition{
+		Type:    "Foo",
+		Status:  corev1.ConditionTrue,
+		Message: "Something something foo",
+	}
+	readyCond := apis.Condition{
+		Type:    apis.ConditionType(topLevelCond),
+		Status:  corev1.ConditionTrue,
+		Message: "Something something bar",
+	}
+
+	return &duckv1.KResource{
+		ObjectMeta: metav1.ObjectMeta{
+			Generation: 42,
+		},
+
+		Status: duckv1.Status{
+			ObservedGeneration: 0,
+			Conditions:         duckv1.Conditions{fooCond, readyCond},
+		},
+	}
+}
+
+func TestPreProcessResetsReady(t *testing.T) {
+	testCases := []struct {
+		name                      string
+		initTopLevelCond          string
+		expectedTopLevelCondition apis.ConditionType
+	}{{
+		name:                      "top level Ready",
+		initTopLevelCond:          "Ready",
+		expectedTopLevelCondition: apis.ConditionReady,
+	}, {
+		name:                      "top level Succeeded",
+		initTopLevelCond:          "Succeeded",
+		expectedTopLevelCondition: apis.ConditionSucceeded,
+	}}
+
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			resource := makeResource(test.initTopLevelCond)
+			krShape := duckv1.KRShaped(resource)
+
+			PreProcessReconcile(context.Background(), krShape)
+
+			if rc := resource.Status.GetCondition(test.expectedTopLevelCondition); rc.Status != "Unknown" {
+				t.Errorf("Expected unchanged ready status got=%s want=Unknown", rc.Status)
+			}
+		})
+	}
+}
+
+func TestPostProcessReconcileBumpsGeneration(t *testing.T) {
+	resource := makeResource("Ready")
+
+	krShape := duckv1.KRShaped(resource)
+	PostProcessReconcile(context.Background(), krShape)
+
+	if resource.Status.ObservedGeneration != resource.Generation {
+		t.Errorf("Expected observed generation bump got=%d want=%d", resource.Status.ObservedGeneration, resource.Generation)
+	}
+
+	if krShape.GetStatus().ObservedGeneration != krShape.GetObjectMeta().GetGeneration() {
+		t.Errorf("Expected observed generation bump got=%d want=%d", resource.Status.ObservedGeneration, resource.Generation)
+	}
+}