Move revision condition manipulation to _types.go by mattmoor · Pull Request #1118 · knative/serving

mattmoor · 2018-06-08T23:54:48Z

This is the Revision equivalent of #1113

This contains two other unrelated changes:

Condense the object construction in revision_test.go
Reorder the helloworld yaml to create the configuration first.

The reason for the second "unrelated" change is that I was seeing the route controller marking the Route as "AllTrafficAssigned = False" because the configuration wasn't created yet.

mattmoor · 2018-06-08T23:57:25Z

Oh, this also makes setCondition private on these types, which means population of Ready should be fully managed auto-magically. I am changing this to [WIP] because I'd like to add unit test coverage for all of the condition manipulation logic.

knative-metrics-robot · 2018-06-09T00:35:37Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	82.8%	38.1%	-44.7
pkg/apis/serving/v1alpha1/revision_types_test.go	82.8%	38.1%	-44.7
pkg/apis/serving/v1alpha1/route_types.go	30.4%	35.0%	4.6
pkg/apis/serving/v1alpha1/route_types_test.go	30.4%	35.0%	4.6
pkg/controller/route/route.go	78.9%	78.8%	-0.1
pkg/controller/revision/revision.go	74.8%	73.7%	-1.2
pkg/controller/revision/revision_test.go	74.8%	73.7%	-1.2

*TestCoverage feature is being tested, do not rely on any info here yet

knative-metrics-robot · 2018-06-09T01:35:13Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	82.8%	90.5%	7.7
pkg/apis/serving/v1alpha1/revision_types_test.go	82.8%	90.5%	7.7
pkg/apis/serving/v1alpha1/route_types.go	30.4%	85.0%	54.6
pkg/apis/serving/v1alpha1/route_types_test.go	30.4%	85.0%	54.6
pkg/controller/route/route.go	78.9%	78.8%	-0.1
pkg/controller/revision/revision.go	74.8%	73.7%	-1.2
pkg/controller/revision/revision_test.go	74.8%	73.7%	-1.2

*TestCoverage feature is being tested, do not rely on any info here yet

mattmoor · 2018-06-09T01:51:19Z

/assign @jonjohnsonjr
/assign @evankanderson

mattmoor · 2018-06-11T18:13:36Z

/assign @tcnghia

Jon's travelling.

tcnghia · 2018-06-11T18:47:27Z

 	}
 	// Don't modify the informer's copy.
 	rev = rev.DeepCopy()
+	rev.Status.InitializeConditions()


does this set all existing conditions on the Status to Unknown? are there cases where we would want to retain existing conditions?

This only populates missing Conditions.

tcnghia · 2018-06-11T18:51:05Z

-// TODO(mattmoor): Use a method on the Build type.
 func isBuildDone(rev *v1alpha1.Revision) (done, failed bool) {
 	if rev.Spec.BuildName == "" {
 		return true, false


personally I'd find it more readable to do

done,failed = true, false return

but the function is small enough so you don't need to do that if you don't want to.

I'll tweak this a bit further

tcnghia · 2018-06-11T18:53:25Z

 	if getIsServiceReady(endpoint) {
 		logger.Infof("Endpoint %q is ready", eName)
-		if err := c.markRevisionReady(ctx, rev); err != nil {
+		rev.Status.MarkResourcesAvailable()


looks like these two always go together? I wonder if they can be the same condition?

Yeah, I was struggling with this myself. I opened #1137 for this discussion.

This is the Revision equivalent of #1113 This contains two other unrelated changes: 1. Condense the object construction in `revision_test.go` 1. Reorder the `helloworld` yaml to create the configuration first. The reason for the second "unrelated" change is that I was seeing the route controller marking the Route as "AllTrafficAssigned = False" because the configuration wasn't created yet.

Eliding conditions can come later, it's a premature optimization, and I'm not sure we're handling conditions well enough to do it reliably yet.

knative-metrics-robot · 2018-06-11T19:31:48Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	82.8%	90.5%	7.7
pkg/apis/serving/v1alpha1/revision_types_test.go	82.8%	90.5%	7.7
pkg/apis/serving/v1alpha1/route_types.go	30.4%	85.0%	54.6
pkg/apis/serving/v1alpha1/route_types_test.go	30.4%	85.0%	54.6
pkg/controller/route/route.go	78.9%	78.8%	-0.1
pkg/controller/revision/revision.go	74.8%	73.6%	-1.3
pkg/controller/revision/revision_test.go	74.8%	73.6%	-1.3

*TestCoverage feature is being tested, do not rely on any info here yet

tcnghia · 2018-06-11T19:35:46Z

thanks a lot for doing this refactoring.

/lgtm
/approve

google-prow-robot · 2018-06-11T19:35:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, tcnghia

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~~ [mattmoor]

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

This is the Service equivalent of #1113, #1118, #1132 However, Service's condition are largely not implemented, so mostly deferred to: #1134

google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 8, 2018

mattmoor changed the title ~~Move revision condition manipulation to _types.go~~ [WIP] Move revision condition manipulation to _types.go Jun 8, 2018

google-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2018

google-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 9, 2018

mattmoor changed the title ~~[WIP] Move revision condition manipulation to _types.go~~ Move revision condition manipulation to _types.go Jun 9, 2018

google-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2018

google-prow-robot assigned evankanderson and jonjohnsonjr Jun 9, 2018

This was referenced Jun 9, 2018

covbot reports should not include _test.go files #1119

Closed

Best practices for updating status #1107

Closed

Move configuration condition manipulation into _types.go #1132

Merged

Move service condition manipulation into _types.go #1135

Merged

google-prow-robot assigned tcnghia Jun 11, 2018

tcnghia reviewed Jun 11, 2018

View reviewed changes

mattmoor mentioned this pull request Jun 11, 2018

Do we need both ContainerHealthy / ResourcesAvailable conditions? #1137

Closed

mattmoor added 5 commits June 11, 2018 19:27

Update the conditions spec to eliminate wording about elision.

80668f7

Eliding conditions can come later, it's a premature optimization, and I'm not sure we're handling conditions well enough to do it reliably yet.

Fix test breakages due to setCondition becoming private.

2a40061

Add unit test coverage of the condition logic.

5638571

Clarify isBuildDone through local variable names.

c27e365

mattmoor force-pushed the revision-conditions branch from af209cd to c27e365 Compare June 11, 2018 19:29

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2018

google-prow-robot merged commit 3ec60bc into knative:master Jun 11, 2018

mattmoor deleted the revision-conditions branch June 11, 2018 20:02

google-prow-robot pushed a commit that referenced this pull request Jun 11, 2018

This is the Configuration equivalent of #1113, #1118 (#1132)

e6ea8c7

google-prow-robot pushed a commit that referenced this pull request Jun 11, 2018

Move service condition manipulation into _types.go (#1135)

2fc4269

This is the Service equivalent of #1113, #1118, #1132 However, Service's condition are largely not implemented, so mostly deferred to: #1134

Conversation

mattmoor commented Jun 8, 2018

Uh oh!

mattmoor commented Jun 8, 2018

Uh oh!

knative-metrics-robot commented Jun 9, 2018

Uh oh!

knative-metrics-robot commented Jun 9, 2018

Uh oh!

mattmoor commented Jun 9, 2018

Uh oh!

mattmoor commented Jun 11, 2018

Uh oh!

tcnghia Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

mattmoor Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

tcnghia Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

mattmoor Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

tcnghia Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

mattmoor Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Jun 11, 2018

Uh oh!

tcnghia commented Jun 11, 2018

Uh oh!

google-prow-robot commented Jun 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants