Update condition type in spec and do not assert against reason (knative#903)

* Some spec docs were using `RolloutComplete` for the condition type,
  however as errors.md documents and as discussed in knative#351, the type
  should actually be `Ready`
* The conformance tests were asserting against the condition `Reason`
  but as per knative#351 and errors.md, values in this field are for the
  user to consume and should be transpart to client applications.
* errors.md didn't make the latter as clear as mattmoor@'s comment
  in knative#351 so I copied some of his comment into the doc

Also reorganized some of errors.md to make it easier to identify,
by field, what requirements are.

Author: bobcatfish

docs/spec/errors.md

@@ -1,88 +1,111 @@
 # Error Conditions and Reporting
 
-Elafros uses the standard Kubernetes API pattern for reporting
-configuration errors and current state of the system by writing the
+Elafros uses [the standard Kubernetes API
+pattern](https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#typical-status-properties)
+for reporting configuration errors and current state of the system by writing the
 report in the `status` section. There are two mechanisms commonly used
-in status:
+in `status`:
 
-* conditions represent true/false statements about the current state
-  of the resource.
+* **Conditions** represent true/false statements about the current
+  state of the resource.
 
-* other fields may provide status on the most recently retrieved state
+* **Other fields** may provide status on the most recently retrieved state
   of the system as it relates to the resource (example: number of
   replicas or traffic assignments).
 
 Both of these mechanisms often include additional data from the
 controller such as `observedGeneration` (to determine whether the
 controller has seen the latest updates to the spec).
 
+## Conditions
+
 Conditions provide an easy mechanism for client user interfaces to
 indicate the current state of resources to a user. Elafros resources
-should follow these patterns:
-
-1. Each resource should define a small number of success conditions as
-   Types. This should bias towards fewer than 5 high-level progress
-   categories which are separate and meaningful for customers. For a
-   Revision, these might be `BuildSucceeded`, `ResourcesAvailable` and
-   `ContainerHealthy`.
-2. Where it makes sense, resources should define a top-level "happy
-   state" condition type which indicates that the resource is set up
-   correctly and ready to serve. For long-running resources, this
-   condition type should be `Ready`. For objects which run to completion,
-   the condition type should be `Succeeded`.
-3. Each condition's status should be one of:
-   * `Unknown` when the controller is actively working to achieve the
-     condition.
-   * `False` when the reconciliation has failed. This should be a terminal
-     failure state until user action occurs.
-   * `True ` when the reconciliation has succeeded. Once all transition
-     conditions have succeeded, the "happy state" condition should be set
-     to `True`.
-     
-   Type names should be chosen such that these interpretations are clear:
-   
-   > `BuildSucceeded` works because `True` = success and `False` = failure.
-   
-   > `BuildCompleted` does not, because `False` could mean "in-progress".
-   
-   Conditions may also be omitted entirely if reconciliation has been
-   skipped. When all conditions have succeeded, the "happy state"
-   should clear other conditions for output legibility. Until the
-   "happy state" is set, conditions should be persisted for the
-   benefit of UI tools representing progress on the outcome.
-   
-4. Conditions with a status of `False` will also supply additional details
-   about the failure in the "Reason" and "Message" sections -- both of
-   these should be considered to have unlimited cardinality, unlike
-   Type. If a resource has a "happy state" type, it will surface the
-   `Reason` and `Message` from the first failing sub Condition.
+should follow [the k8s API conventions for
+`condition`](https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#typical-status-properties)
+and the patterns described in this section.
+
+### Elafros condition `type`
+
+Each resource should define a small number of success conditions as
+`type`s. This should bias towards fewer than **5** high-level progress
+categories which are separate and meaningful for customers. For a
+Revision, these might be `BuildSucceeded`, `ResourcesAvailable` and
+`ContainerHealthy`.
+
+Where it makes sense, resources should define a top-level "happy
+state" condition `type` which indicates that the resource is set up
+correctly and ready to serve.
+
+* For long-running resources, this condition `type` should be
+  `Ready`.
+* For objects which run to completion, the condition `type` should
+  be `Succeeded`.
+
+### Elafros condition `status`
+
+Each condition's `status` should be one of:
+
+* `Unknown` when the controller is actively working to achieve the
+  condition.
+* `False` when the reconciliation has failed. This should be a terminal
+  failure state until user action occurs.
+* `True` when the reconciliation has succeeded. Once all transition
+  conditions have succeeded, the "happy state" condition should be set
+  to `True`.
+
+Type names should be chosen such that these interpretations are clear:
+
+* `BuildSucceeded` works because `True` = success and `False` = failure.
+* `BuildCompleted` does not, because `False` could mean "in-progress".
+
+Conditions may also be omitted entirely if reconciliation has been
+skipped. When all conditions have succeeded, the "happy state"
+should clear other conditions for output legibility. Until the
+"happy state" is set, conditions should be persisted for the
+benefit of UI tools representing progress on the outcome.
+
+Conditions with a status of `False` will also supply additional details
+about the failure in [the "Reason" and "Message" sections](#condition-reason-and-message).
+
+### Elafros condition `reason` and `message`
+
+The fields `reason` and `message` should be considered to have unlimited
+cardinality, unlike [`type`](#condition-type) and [`status`](#condition-status).
+If a resource has a "happy state" [`type`](#condition-type), it will surface the
+`reason` and `message` from the first failing sub Condition.
+
+The values `reason` takes on (while camelcase words) should be treated as opaque.
+Clients shouldn't programmatically act on their values, but bias towards using
+`reason` as a terse explanation of the state for end-users, whereas `message`
+is the long-form of this.
+
+## Example scenarios
 
 Example user and system error scenarios are included below along with
 how the status is presented to CLI and UI tools via the API.
 
 * [Deployment-Related Failures](#deployment-related-failures)
-   * [Revision failed to become Ready](#revision-failed-to-become-ready)
-   * [Build failed](#build-failed)
-   * [Resource exhausted while creating a revision](#resource-exhausted-while-creating-a-revision)
-   * [Container image not present in repository](#container-image-not-present-in-repository)
-   * [Container image fails at startup on Revision](#container-image-fails-at-startup-on-revision)
-   * [Deployment progressing slowly/stuck](#deployment-progressing-slowly-stuck)
+  * [Revision failed to become Ready](#revision-failed-to-become-ready)
+  * [Build failed](#build-failed)
+  * [Resource exhausted while creating a revision](#resource-exhausted-while-creating-a-revision)
+  * [Container image not present in repository](#container-image-not-present-in-repository)
+  * [Container image fails at startup on Revision](#container-image-fails-at-startup-on-revision)
+  * [Deployment progressing slowly/stuck](#deployment-progressing-slowly-stuck)
 * [Routing-Related Failures](#routing-related-failures)
-   * [Traffic not assigned](#traffic-not-assigned)
-   * [Revision not found by Route](#revision-not-found-by-route)
-   * [Configuration not found by Route](#configuration-not-found-by-route)
-   * [Latest Revision of a Configuration deleted](#latest-revision-of-a-configuration-deleted)
-   * [Traffic shift progressing slowly/stuck](#traffic-shift-progressing-slowly-stuck)
-
+  * [Traffic not assigned](#traffic-not-assigned)
+  * [Revision not found by Route](#revision-not-found-by-route)
+  * [Configuration not found by Route](#configuration-not-found-by-route)
+  * [Latest Revision of a Configuration deleted](#latest-revision-of-a-configuration-deleted)
+  * [Traffic shift progressing slowly/stuck](#traffic-shift-progressing-slowly-stuck)
 
-# Deployment-Related Failures
+## Deployment-Related Failures
 
 The following scenarios will generally occur when attempting to deploy
 changes to the software stack by updating the Service or Configuration
 resources to cause a new Revision to be created.
 
-
-## Revision failed to become Ready
+### Revision failed to become Ready
 
 If the latest Revision fails to become `Ready` for any reason within
 some reasonable timeframe, the Configuration and Service should signal
@@ -92,6 +115,7 @@ message from the `Ready` condition on the Revision.
 ```http
 GET /api/elafros.dev/v1alpha1/namespaces/default/configurations/my-service
 ```
+
 ```yaml
 ...
 status:
@@ -107,6 +131,7 @@ status:
 ```http
 GET /api/elafros.dev/v1alpha1/namespaces/default/services/my-service
 ```
+
 ```yaml
 ...
 status:
@@ -121,8 +146,7 @@ status:
     meassage: "Build Step XYZ failed with error message: $LASTLOGLINE"
 ```
 
-
-## Build failed
+### Build failed
 
 If the Build steps failed while creating a Revision, you can examine
 the `Failed` condition on the Build or the `BuildSucceeded` condition
@@ -134,6 +158,7 @@ build.
 ```http
 GET /apis/build.dev/v1alpha1/namespaces/default/builds/build-1acub3
 ```
+
 ```yaml
 ...
 status:
@@ -149,6 +174,7 @@ status:
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/revisions/abc
 ```
+
 ```yaml
 ...
 status:
@@ -163,8 +189,7 @@ status:
     message: "Step XYZ failed with error message: $LASTLOGLINE"
 ```
 
-
-## Resource exhausted while creating a revision
+### Resource exhausted while creating a revision
 
 Since a Revision is only metadata, the Revision will be created, but
 will have a condition indicating the underlying failure, possibly
@@ -175,6 +200,7 @@ into the underlying resources in the hosting environment.
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/revisions/abc
 ```
+
 ```yaml
 ...
 status:
@@ -189,8 +215,7 @@ status:
     message: "The controller could not create a deployment named ela-abc-e13ac."
 ```
 
-
-## Container image not present in repository
+### Container image not present in repository
 
 Revisions might be created while a Build is still creating the
 container image or uploading it to the repository. If the build is
@@ -208,6 +233,7 @@ the original docker image is deleted.
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/revisions/abc
 ```
+
 ```yaml
 ...
 status:
@@ -222,8 +248,7 @@ status:
     message: "Unable to fetch image 'gcr.io/...': <literal error>"
 ```
 
-
-## Container image fails at startup on Revision
+### Container image fails at startup on Revision
 
 Particularly for development cases with interpreted languages like
 Node or Python, syntax errors might only be caught at container
@@ -241,6 +266,7 @@ be used to fetch the logs for the failed process.
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/revisions/abc
 ```
+
 ```yaml
 ...
 status:
@@ -256,8 +282,7 @@ status:
     message: "Container failed with: SyntaxError: Unexpected identifier"
 ```
 
-
-## Deployment progressing slowly/stuck
+### Deployment progressing slowly/stuck
 
 See [the kubernetes documentation for how this is handled for
 Deployments](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#failed-deployment). For
@@ -275,6 +300,7 @@ might attempt to make progress even after the deadline.
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/revisions/abc
 ```
+
 ```yaml
 ...
 status:
@@ -285,16 +311,14 @@ status:
     message: "Did not pass readiness checks in 120 seconds."
 ```
 
-
-# Routing-Related Failures
+## Routing-Related Failures
 
 The following scenarios are most likely to occur when attempting to
 roll out a change by shifting traffic to a new Revision. Some of these
 conditions can also occur under normal operations due to (for example)
 operator error causing live resources to be deleted.
 
-
-## Traffic not assigned
+### Traffic not assigned
 
 If some percentage of traffic cannot be assigned to a live
 (materialized or scaled-to-zero) Revision, the Route will report the
@@ -305,6 +329,7 @@ the first Revision is unable to serve:
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/routes/my-service
 ```
+
 ```yaml
 ...
 status:
@@ -322,6 +347,7 @@ status:
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/services/my-service
 ```
+
 ```yaml
 ...
 status:
@@ -339,8 +365,7 @@ status:
     message: "Container failed with: SyntaxError: Unexpected identifier"
 ```
 
-
-## Revision not found by Route
+### Revision not found by Route
 
 If a Revision is referenced in a Route's `spec.traffic`, and the Revision
 cannot be found, the `AllTrafficAssigned` condition will be marked as False
@@ -350,6 +375,7 @@ Route's  `status.traffic`.
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/routes/my-service
 ```
+
 ```yaml
 ...
 status:
@@ -368,8 +394,7 @@ status:
     message: "Revision 'qyzz' referenced in traffic not found"
 ```
 
-
-## Configuration not found by Route
+### Configuration not found by Route
 
 If a Route references the `latestReadyRevisionName` of a Configuration
 and the Configuration cannot be found, the `AllTrafficAssigned` condition
@@ -379,6 +404,7 @@ Revision will be omitted from the Route's  `status.traffic`.
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/routes/my-service
 ```
+
 ```yaml
 ...
 status:
@@ -394,8 +420,7 @@ status:
     message: "Configuration 'abc' referenced in traffic not found"
 ```
 
-
-## Latest Revision of a Configuration deleted
+### Latest Revision of a Configuration deleted
 
 If the most recent Revision is deleted, the Configuration will set
 `LatestRevisionReady` to False.
@@ -409,6 +434,7 @@ set the `AllTrafficAssigned` condition to False with reason
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/configurations/my-service
 ```
+
 ```yaml
 ...
 metadata:
@@ -426,8 +452,7 @@ status:
   observedGeneration: 1234
 ```
 
-
-## Traffic shift progressing slowly/stuck
+### Traffic shift progressing slowly/stuck
 
 Similar to deployment slowness, if the transfer of traffic (either via
 gradual or abrupt rollout) takes longer than a certain timeout to
@@ -437,6 +462,7 @@ True, but the reason will be set to `ProgressDeadlineExceeded`.
 ```http
 GET /apis/elafros.dev/v1alpha1/namespaces/default/routes/my-service
 ```
+
 ```yaml
 ...
 status:
@@ -451,4 +477,4 @@ status:
     reason: ProgressDeadlineExceeded
     # reason is a short status, message provides error details
     message: "Unable to update traffic split for more than 120 seconds."
-```
+```

docs/spec/normative_examples.md

@@ -217,7 +217,7 @@ status:
   - revisionName: def
     percent: 25
   conditions:
-  - type: RolloutComplete
+  - type: Ready
     status: False
 ```
 
@@ -243,7 +243,7 @@ status:
   - revisionName: def
     percent: 100
   conditions:
-  - type: RolloutComplete
+  - type: Ready
     status: True
   ...
 ```
@@ -475,7 +475,7 @@ status:
     percent: 100
 
   conditions:
-  - type: RolloutComplete
+  - type: Ready
     status: True
 
   observedGeneration: 2145
@@ -713,7 +713,7 @@ status:
     name: next # addressable as next.my-service.default.mydomain.com
     percent: 0
   conditions:
-  - type: RolloutComplete
+  - type: Ready
     status: True
 ```
 
@@ -787,7 +787,7 @@ status:
     name: next # addressable as next.my-service.default.mydomain.com
     percent: 0
   conditions:
-  - type: RolloutComplete
+  - type: Ready
     status: True
 ```
 

docs/spec/spec.md

@@ -87,7 +87,7 @@ status:
   - ...
 
   conditions:  # See also the [error conditions documentation](errors.md)
-  - type: RolloutComplete
+  - type: Ready
     status: True
   - type: AllTrafficAssigned
     status: True

test/states.go

@@ -53,18 +53,10 @@ func IsRevisionReady(revisionName string) func(r *v1alpha1.Revision) (bool, erro
 			if r.Status.Conditions[0].Type != v1alpha1.RevisionConditionType("Ready") {
 				return true, fmt.Errorf("Expected Revision to have a \"Ready\" status but only had %s", r.Status.Conditions[0].Type)
 			}
-			if r.Status.Conditions[0].Status == corev1.ConditionStatus("Unknown") {
-				if r.Status.Conditions[0].Reason != "Deploying" {
-					return true, fmt.Errorf("If the Revision isn't ready the reason should be to be \"Deploying\" but was %s", r.Status.Conditions[0].Reason)
-				}
-			} else {
-				if r.Status.Conditions[0].Status != corev1.ConditionStatus("True") {
-					return true, fmt.Errorf("Expected Revision Status Condition Status to be True or Unknown but was %s", r.Status.Conditions[0].Status)
-				}
-				if r.Status.Conditions[0].Reason != "ServiceReady" {
-					return true, fmt.Errorf("If the Revision is ready the Reason should be \"ServiceReady\" but was %s", r.Status.Conditions[0].Status)
-				}
+			if r.Status.Conditions[0].Status == corev1.ConditionTrue {
 				return true, nil
+			} else if r.Status.Conditions[0].Status != corev1.ConditionUnknown {
+				return true, fmt.Errorf("Expected Revision Status Condition Status to be True or Unknown but was %s", r.Status.Conditions[0].Status)
 			}
 		}
 		return false, nil