feat: support populating KongUpstreamPolicy status for ServiceFacade #5428
Conversation
czeslavo
changed the title
czeslavo
added
area/feature
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5428 +/- ##
=======================================
+ Coverage 0 69.6% +69.6%
=======================================
Files 0 176 +176
Lines 0 22507 +22507
=======================================
+ Hits 0 15670 +15670
- Misses 0 5912 +5912
- Partials 0 925 +925 ☔ View full report in Codecov by Sentry. |
czeslavo
force-pushed
the
feat/support-service-facade-ancestor
branch
from
835bc6d
to
37b16f8
Compare
czeslavo
force-pushed
the
feat/support-service-facade-ancestor
branch
2 times, most recently
from
3a1e0ff
to
e327869
Compare
czeslavo
removed
the
do not merge
czeslavo
changed the title
czeslavo
changed the title
czeslavo
force-pushed
the
feat/support-service-facade-ancestor
branch
from
e327869
to
7287d8b
Compare
| groupIsCoreOrNil := br.Group == nil || *br.Group == "core" | ||
| kindIsServiceOrNil := br.Kind == nil || *br.Kind == "Service" | ||
| // We only support core Services. | ||
| return groupIsCoreOrNil && kindIsServiceOrNil |
There was a problem hiding this comment.
This will return true for br.Group == nil and br.Kind == nil (and also for pairs like br.Group == "core" and br.Kind == nil). Is that expected?
There was a problem hiding this comment.
The nil case for Kind should never happen as it defaults on the API level to Service. I think that we can safely consider nil to be treated as Service if it would happen for any reason.
For Group, the specification says When unspecified or empty string, core API group is inferred. so we also consider the nil case to be core API group.
There was a problem hiding this comment.
It makes sense reading your comment 👍 But without that comment I wouldn't be so sure about it 🙃 Do you think it might be worth adding it?
There was a problem hiding this comment.
Added to #5444 as well. 👍
| sort.Slice(ancestorsStatus, func(i, j int) bool { | ||
| return ancestorsStatus[i].creationTimestamp.Before(&ancestorsStatus[j].creationTimestamp) | ||
| }) | ||
| if len(ancestorsStatus) > maxNAncestors { |
There was a problem hiding this comment.
maxNAncestors feels like an implementation detail that users should be aware of. We're not documenting this but this is being enforced on the Gateway API level in gatewayv1alpha2.PolicyStatus. Perhaps it's worth documenting it in a comment and in the log message?
| // ----------------------------------------------------------------------------- | ||
| // KongUpstreamPolicy Controller - Helpers | ||
| // ----------------------------------------------------------------------------- | ||
| func (r *KongUpstreamPolicyReconciler) buildPolicyStatus( |
There was a problem hiding this comment.
How about a godoc for this func?
What this PR does / why we need it:
As
KongUpstreamPolicycan be attached toKongServiceFacadeas well as to aService, we should support populating status for it. This PR adds support forAcceptedcondition.Programmedcondition forKongServiceFacadewill be added along withServicein #5249.Which issue this PR fixes:
Adding
KongServiceFacadeparity before #5249.PR Readiness Checklist:
Complete these before marking the PR as
ready to review:CHANGELOG.mdrelease notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR