add conformance test for labels by nader-ziada · Pull Request #2039 · knative/serving

nader-ziada · 2018-09-17T19:12:07Z

Fixes #2021

Proposed Changes

Add new conformance test to check labels propagation in serving objects

Release Note

NONE

nader-ziada · 2018-09-17T19:42:11Z

any comments about this test? anything else we need to cover?
@dprotaso @mattmoor @evankanderson @shashwathi

nader-ziada · 2018-09-18T13:13:59Z

/assign @mattmoor

evankanderson · 2018-09-18T18:03:19Z

/assign @evankanderson

I'll take a look shortly. Do you want to also write up a change to the spec, or do you want me to try to write that up? I'd be happy to help with wordsmithing if you want to make the spec change (Probably https://github.com/knative/serving/blob/master/docs/spec/spec.md and maybe https://github.com/knative/serving/blob/master/docs/runtime-contract.md#process).

evankanderson

Looking further at this test, this should be spelled out better in the spec document: we have a completely different set of labels on the object right now. (And the "type" label can probably go, as it was advisory rather than mandatory.)

It may also be worth adding a small amount of text above the sample YAML explaining how each objects labels are derived from its relations.

evankanderson · 2018-09-18T17:43:02Z

+	var imagePath = test.ImagePath("helloworld")
+
+	var names test.ResourceNames
+	names.Service = test.AppendRandomString("pizzaplanet-service", logger)


Prefer:

names := test.ResourceNames{Service: test.AppendRandomString("pizzaplanet-service", logger)

evankanderson · 2018-09-18T17:46:28Z

+	// Add test case specific name to its own logger.
+	logger := logging.GetContextLogger("TestLabelsPropagation")
+
+	var imagePath = test.ImagePath("helloworld")


Move this below the initialization of "names" so that the setup of clients and names are as close as possible to the deferred tearDown.

evankanderson · 2018-09-18T18:05:38Z

+	names.Config = serviceresourcenames.Configuration(svc)
+
+	logger.Info("The Service will be updated with the name of the Revision once it is created")
+	var revisionName string


I don't think we need this revisionName here, since we overwrite it on line 62?

evankanderson · 2018-09-18T18:12:04Z

+		}
+		return false, nil
+	}, "ServiceUpdatedWithRevision")
+	revisionName, err = waitForServiceLatestCreatedRevision(clients, names)


Why do we have both WaitForServiceState and waitForServiceLatestCreatedRevision?

evankanderson · 2018-09-18T18:21:50Z

+			revisionName = s.Status.LatestCreatedRevisionName
+			return true, nil
+		}
+		return false, nil


If you don't need to capture revisionName, this can be:

return s.Status.LatestCreatedRevisionname != names.Revision, nil

evankanderson · 2018-09-18T18:22:45Z

+	if err != nil {
+		t.Fatalf("Service %s was not updated with the new revision: %v", names.Service, err)
+	}
+	names.Revision = revisionName


Why not assign to names.Revision on line 62?

evankanderson · 2018-09-18T18:23:39Z

+		t.Fatalf("The Service %s was not marked as Ready to serve traffic to Revision %s: %v", names.Service, names.Revision, err)
+	}
+
+	logger.Info("Validate Labels on Revision Object")


Add a comment here that the setup is complete, and what we're intending to test (possibly as a reference to the spec)?

evankanderson · 2018-09-18T18:25:40Z

+		}
+	} else {
+		t.Fatalf("Failed to get configuration name from Revision label")
+	}


Totally optional: golang will return a empty value from a map lookup if you use the single-assignment form. Assuming that you know that names.Config is non-empty, you could write this as:

if revision.Labels["serving.knative.dev/configuration"] != names.Config { t.Fatalf(...) }

Also, I'd suggest using t.Errorf() here, to fail the test but allow it to keep executing. This will allow additional conditions to be tested and debugged at one time.

This might also be an opportunity for a table-driven test; ala:

struct TestCase { Name string Fetcher func(string, metav1.Options) (interface, error) // Must be an object Labels map[string]string } expectedLabels := []TestCase{ { Name: names.Revision, Fetcher: clients.ServingClient.Revisions.Get, Labels: { "serving.knative.dev/configuration": names.Config, "serving.knative.dev/service": names.Service, }, }, ... }

@evankanderson

I believe the table test would not work as your example above because Go doesn't allow covariant return types (language: covariance support golang/go#7512) as mentioned in a similar conversation in PR Reconciler configs are snapshotted at the start of each reconciliation #2009

The Fetcher func can't return a generic interface, and the needed objects here do not confirm to any common interface.

The other option is to use kubernetes dynamic client to return *unstructured.Unstructured object and get the Labels from that.

I feel this would be complicating this test too much since we have to change the client used, WDYT?

Thanks, I'd missed that the conversion between clients.ServingClient.Revisions.Get and the Fetcher wouldn't work. Given that, I agree that this is the most reasonable formulation.

nader-ziada · 2018-09-19T19:14:16Z

@evankanderson addressed comments

evankanderson

/approve

A few small corrections, but this looks good. Thanks!

evankanderson · 2018-09-20T18:42:19Z

  namespace: default
  labels:
-    knative.dev/type: ...  # +optional convention: function|app
+    knative.dev/service: ...  # name of the service   


Mention this is filled in automatically.

evankanderson · 2018-09-20T18:42:30Z

  namespace: default
-
+  labels:
+    knative.dev/service: ...  # name of the service


evankanderson · 2018-09-20T18:47:21Z

+		t.Fatalf("The Service %s was not marked as Ready to serve traffic to Revision %s: %v", names.Service, names.Revision, err)
+	}
+
+	// Now that the serive is ready, we can validate the Labels on the Revision, Configuration


"serive" -> Service.

evankanderson · 2018-09-20T18:48:26Z

+	}
+
+	if revision.Labels["serving.knative.dev/configuration"] != names.Config {
+		t.Errorf("Expect confguration name in revision label %q but got %q ", names.Config, revision.Labels["serving.knative.dev/configuration"])


"Configuration"

evankanderson · 2018-09-20T18:49:43Z

+	}
+
+	if route.Labels["serving.knative.dev/service"] != names.Service {
+


Extra line here.

evankanderson · 2018-09-20T18:59:20Z

+		}
+	} else {
+		t.Fatalf("Failed to get configuration name from Revision label")
+	}


Thanks, I'd missed that the conversion between clients.ServingClient.Revisions.Get and the Fetcher wouldn't work. Given that, I agree that this is the most reasonable formulation.

nader-ziada · 2018-09-20T19:49:03Z

@evankanderson addressed comments

evankanderson

/lgtm
/approve

Thanks for doing this, and your patience with my reviews!

knative-prow-robot · 2018-09-20T23:28:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, pivotal-nader-ziada

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:

~~OWNERS~~ [evankanderson]

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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 17, 2018

knative-prow-robot requested review from srinivashegde86 and steuhs September 17, 2018 19:12

knative-prow-robot assigned mattmoor Sep 18, 2018

knative-prow-robot assigned evankanderson Sep 18, 2018

evankanderson reviewed Sep 18, 2018

View reviewed changes

evankanderson changed the title ~~add conformance test for lables~~ add conformance test for labels Sep 18, 2018

mattmoor removed their assignment Sep 19, 2018

nader-ziada added 2 commits September 19, 2018 10:45

add conformance test for lables

65210f7

simplify test, update spec doc

286036d

evankanderson reviewed Sep 20, 2018

View reviewed changes

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

minor changes for comments

95b42d9

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

evankanderson reviewed Sep 20, 2018

View reviewed changes

knative-prow-robot merged commit 984c181 into knative:master Sep 20, 2018

nader-ziada mentioned this pull request Sep 21, 2018

Add service label to revisions #2011

Closed

		}

		if route.Labels["serving.knative.dev/service"] != names.Service {

Conversation

nader-ziada commented Sep 17, 2018

Proposed Changes

Uh oh!

nader-ziada commented Sep 17, 2018

Uh oh!

nader-ziada commented Sep 18, 2018

Uh oh!

evankanderson commented Sep 18, 2018

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nader-ziada commented Sep 19, 2018

Uh oh!

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

nader-ziada commented Sep 20, 2018

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Sep 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants