Updating k8s visibility will update the specific subroute's visibility. #4560
Conversation
knative-prow-robot
added
size/XXL
knative-prow-robot
requested review from
chizhg,
lichuqiang,
markusthoemmes,
tcnghia and
vagababov
andrew-su
force-pushed
the
minify/subroutes/3
branch
4 times, most recently
from
c2fe3d9 to
2781685
Compare
andrew-su
force-pushed
the
minify/subroutes/3
branch
3 times, most recently
from
006cc78 to
904ebf9
Compare
andrew-su
force-pushed
the
minify/subroutes/3
branch
from
904ebf9 to
dfe5670
Compare
|
The following is the coverage report on pkg/.
|
andrew-su
changed the title
knative-prow-robot
added
the
do-not-merge/work-in-progress
andrew-su
changed the title
knative-prow-robot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
|
/assign @mattmoor for apis/OWNERS |
|
/approve The change to pkg/apis is just a comment change noting the expanded use of the label. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrew-su, mattmoor, tcnghia The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
/retest |
| } | ||
|
|
||
| // SetVisibility sets the visibility on an ObjectMeta | ||
| func SetVisibility(meta *v1.ObjectMeta, isClusterLocal bool) { |
There was a problem hiding this comment.
@andrew-su Should SetVisibility and SetLabel belong under route? If I'm understanding this correctly, they are not modifying route specific attributes
There was a problem hiding this comment.
In the original implementation this took in a route object. I later switched it to just ObjectMeta. As this is currently the only usages of this is in the route reconciler, I didn't move it. Is there somewhere you think is a better fit?
There was a problem hiding this comment.
@andrew-su ah I see. How about moving labels under pkg/reconciler/service?
There was a problem hiding this comment.
I don't think I agree with moving it to pkg/reconciler/service as that's specific to the knative service; the usage is currently used by the route reconciler, it doesn't make sense to me to reach into another reconciler's package to get this functionality. I can see it living in maybe under pkg. Perhaps even waiting until the functionality is required elsewhere to refactor it out to a more common location.
There was a problem hiding this comment.
@andrew-su I see. Initially I thought it was a bit odd since it's operating on the service object, but was living under pkg/reconciler/route. I can't think of a good destination for it now. Like you said, we can move it to another location in the future
In an effort to split up my larger PR.
This depends on PR #4559
Updating the service's visibility label (or removing it) will propagate to the actual visibility of the subroutes.
Once the service is created, the visibility is only controlled by the service. Updating the route's visibility will only affect new subroute's default visibility.