Repository: knative/pkg
Pull request #1372 (2020-06-02 21:11:06)

Initialize conditions in preprocess
Initialize conditions in preprocess

This is an oft-repeated piece of code that we have in reconcilers. We should ensure conditions are initialized before we reconcile.

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1372#" title="Author self-approved">whaught</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **grantr**
You can assign the PR to them by writing `/assign @grantr` 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:

- **[reconciler/OWNERS](https://github.com/knative/pkg/blob/master/reconciler/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":["grantr"]} -->
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
---- |:------------:|:------------:|:-----:
[reconciler/reconcile_common.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1372/pull-knative-pkg-go-coverage/1267926862764445697/artifacts/line-cov.html#file0) | 84.6% | 85.7% | 1.1

whaught: /assign @n3wscott 
knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1372#pullrequestreview-423082513" title="Approved">n3wscott</a>*, *<a href="https://github.com/knative/pkg/pull/1372#" title="Author self-approved">whaught</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **grantr**
You can assign the PR to them by writing `/assign @grantr` 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:

- **[reconciler/OWNERS](https://github.com/knative/pkg/blob/master/reconciler/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":["grantr"]} -->
whaught: /assign @grantr

Patch:
diff --git a/reconciler/reconcile_common.go b/reconciler/reconcile_common.go
index cc0e2d712..6a32c06f7 100644
--- a/reconciler/reconcile_common.go
+++ b/reconciler/reconcile_common.go
@@ -32,6 +32,8 @@ func PreProcessReconcile(ctx context.Context, resource duckv1.KRShaped) {
 	if newStatus.ObservedGeneration != resource.GetGeneration() {
 		condSet := resource.GetConditionSet()
 		manager := condSet.Manage(newStatus)
+		// Ensure conditions are initialized before we modify.
+		manager.InitializeConditions()
 
 		// Reset Ready/Successful to unknown. The reconciler is expected to overwrite this.
 		manager.MarkUnknown(condSet.GetTopLevelConditionType(), failedGenerationBump, "unsuccessfully observed a new generation")


Test patch:
diff --git a/reconciler/reconcile_common_test.go b/reconciler/reconcile_common_test.go
index 1981e0815..0d75af590 100644
--- a/reconciler/reconcile_common_test.go
+++ b/reconciler/reconcile_common_test.go
@@ -27,19 +27,34 @@ import (
 	duckv1 "knative.dev/pkg/apis/duck/v1"
 )
 
-func makeResource(topLevelCond string) *duckv1.KResource {
+type TestResource struct {
+	metav1.TypeMeta   `json:",inline"`
+	metav1.ObjectMeta `json:"metadata,omitempty"`
+
+	Status duckv1.Status `json:"status"`
+}
+
+func (t *TestResource) GetStatus() *duckv1.Status {
+	return &t.Status
+}
+
+func (*TestResource) GetConditionSet() apis.ConditionSet {
+	return apis.NewLivingConditionSet("Foo", "Bar")
+}
+
+func makeResource() *TestResource {
 	fooCond := apis.Condition{
 		Type:    "Foo",
 		Status:  corev1.ConditionTrue,
 		Message: "Something something foo",
 	}
 	readyCond := apis.Condition{
-		Type:    apis.ConditionType(topLevelCond),
+		Type:    apis.ConditionType(apis.ConditionReady),
 		Status:  corev1.ConditionTrue,
 		Message: "Something something bar",
 	}
 
-	return &duckv1.KResource{
+	return &TestResource{
 		ObjectMeta: metav1.ObjectMeta{
 			Generation: 42,
 		},
@@ -51,37 +66,29 @@ func makeResource(topLevelCond string) *duckv1.KResource {
 	}
 }
 
-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 TestPreProcess(t *testing.T) {
+	resource := makeResource()
+	krShape := duckv1.KRShaped(resource)
+
+	PreProcessReconcile(context.Background(), krShape)
+
+	if rc := resource.Status.GetCondition("Ready"); rc.Status != "Unknown" {
+		t.Errorf("Expected unchanged ready status got=%s want=Unknown", rc.Status)
+	}
+
+	// Ensure Foo is untouched
+	if rc := resource.Status.GetCondition("Foo"); rc.Status != "True" {
+		t.Errorf("Expected dependant condition to remain got=%s want=True", rc.Status)
+	}
+
+	// Ensure Bar is initialized
+	if rc := resource.Status.GetCondition("Bar"); rc.Status != "True" {
+		t.Errorf("Expected conditions to be initialized got=%s want=True", rc.Status)
 	}
 }
 
 func TestPostProcessReconcileBumpsGeneration(t *testing.T) {
-	resource := makeResource("Ready")
+	resource := makeResource()
 
 	krShape := duckv1.KRShaped(resource)
 	PostProcessReconcile(context.Background(), krShape)
diff --git a/test/junit/junit.go b/test/junit/junit.go
index 2e53da618..bc69067be 100644
--- a/test/junit/junit.go
+++ b/test/junit/junit.go
@@ -21,6 +21,8 @@ package junit
 import (
 	"encoding/xml"
 	"fmt"
+	"io/ioutil"
+	"log"
 )
 
 // TestStatusEnum is a enum for test result status
@@ -144,3 +146,24 @@ func UnMarshal(buf []byte) (*TestSuites, error) {
 	}
 	return &testSuites, nil
 }
+
+// CreateXMLErrorMsg outputs a junit testsuite, testname and error message to the destination path
+// in XML format
+func CreateXMLErrorMsg(testSuite, testName, errMsg, dest string) {
+	suites := TestSuites{}
+	suite := TestSuite{Name: testSuite}
+	var errP *string
+	if errMsg != "" {
+		errP = &errMsg
+	}
+	suite.AddTestCase(TestCase{
+		Name:    testName,
+		Failure: errP,
+	})
+	suites.AddTestSuite(&suite)
+	contents, err := suites.ToBytes("", "")
+	if err != nil {
+		log.Fatal(err)
+	}
+	ioutil.WriteFile(dest, contents, 0644)
+}
diff --git a/test/junit/junit_test.go b/test/junit/junit_test.go
index c4b1aa081..21cc64dc7 100644
--- a/test/junit/junit_test.go
+++ b/test/junit/junit_test.go
@@ -19,6 +19,9 @@ limitations under the License.
 package junit
 
 import (
+	"io/ioutil"
+	"os"
+	"path"
 	"testing"
 )
 
@@ -148,3 +151,24 @@ func TestAddTestSuite(t *testing.T) {
 		t.Fatalf("Expected 2, actual %d", len(testSuites.Suites))
 	}
 }
+
+func TestCreateXMLErrorMsg(t *testing.T) {
+	testDir := "test_output"
+	os.RemoveAll(testDir) // clean up in case there were stale side-effects from previous runs
+	if err := os.Mkdir(testDir, 0777); err != nil {
+		t.Fatalf("cannot create directory %q", testDir)
+	}
+	defer os.RemoveAll(testDir) // clean up
+	dest := path.Join(testDir, "TestCreateXMLErrorTestFile")
+	CreateXMLErrorMsg("dummySuite", "dummyTest", "dummyError has occured", dest)
+	expected := `<testsuites><testsuite name="dummySuite" time="0" failures="1" tests="1"><testcase name="dummyTest" time="0" classname=""><failure>dummyError has occured</failure><properties></properties></testcase><properties></properties></testsuite></testsuites>`
+
+	got, err := ioutil.ReadFile(dest)
+	if err != nil {
+		t.Fatalf("cannot read %q, error %v", dest, err)
+	}
+
+	if string(got) != expected {
+		t.Fatalf("expected:\n%q\n, got:\n%q", expected, got)
+	}
+}
diff --git a/testutils/junithelper/main.go b/testutils/junithelper/main.go
index 64be657d8..f5f9a833a 100644
--- a/testutils/junithelper/main.go
+++ b/testutils/junithelper/main.go
@@ -18,8 +18,6 @@ package main
 
 import (
 	"flag"
-	"io/ioutil"
-	"log"
 
 	"knative.dev/pkg/test/junit"
 )
@@ -38,20 +36,5 @@ func main() {
 	flag.StringVar(&dest, "dest", "junit_result.xml", "Where junit xml writes to")
 	flag.Parse()
 
-	suites := junit.TestSuites{}
-	suite := junit.TestSuite{Name: suite}
-	var errP *string
-	if errMsg != "" {
-		errP = &errMsg
-	}
-	suite.AddTestCase(junit.TestCase{
-		Name:    name,
-		Failure: errP,
-	})
-	suites.AddTestSuite(&suite)
-	contents, err := suites.ToBytes("", "")
-	if err != nil {
-		log.Fatal(err)
-	}
-	ioutil.WriteFile(dest, contents, 0644)
+	junit.CreateXMLErrorMsg(suite, name, errMsg, dest)
 }