Refactor route controller to pattern consistent with other controllers.#1444
Refactor route controller to pattern consistent with other controllers.#1444google-prow-robot merged 2 commits intoknative:masterfrom
Conversation
google-prow-robot
added
approved
tcnghia
changed the title
mattmoor
left a comment
mattmoor
left a comment
There was a problem hiding this comment.
This moves us a step closer, but I think has a ways to go to completely follow the same pattern I'd like. This seems like good incremental progress, so let's talk about what's next after this goes in.
Couple nits.
| } | ||
|
|
||
| existing.Status = route.Status | ||
| // TODO: for CRD there's no updatestatus, so use normal update. |
There was a problem hiding this comment.
Please undo this, we don't use UpdateStatus yet (yes, it is poorly worded).
| // If there's nothing to update, just return. | ||
| if reflect.DeepEqual(existing.Status, route.Status) { | ||
| return existing, nil | ||
| } |
There was a problem hiding this comment.
I don't understand this change. I'd leave it for consistency with other updateStatus methods, or remove them all.
| configName := config.Name | ||
| ns := config.Namespace | ||
|
|
||
| func (c *Controller) EnqueueReferringRoute(obj interface{}) { |
There was a problem hiding this comment.
This depends on the Route label on Configurations, which is something we want to be able to relax.
You aren't introducing this, but we should fix this in a future change.
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
This PR move route controller to pattern similar to that of the rest of the controllers, that all resource changes impacting a route will go through the same function.
I am only updating the tests to compile with new code, and have not also refactored the tests. Will follow up in another PR to avoid over-complicating the code review.
Also removing some unused code. In particular, in #1228 I change the activator route to only depends on the Revision's Serving state (whether it is in Reserved or not) and not whether 1->0 is enabled (since we are only dealing with 0->1 activation here). Since then such code has been unused, so I am removing them as well.