Fix AttachedRoutes documentation aligned to count only Accepted Routes #4341
Conversation
Signed-off-by: Davide Salerno <dsalerno@redhat.com>
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: davidesalerno The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @davidesalerno! |
k8s-ci-robot
added
needs-ok-to-test
|
Hi @davidesalerno. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
cncf-cla: yes
|
one missing piece on the docs we have recently changed /ok-to-test |
k8s-ci-robot
added
ok-to-test
davidesalerno
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| // for Listeners with condition Accepted: false but MUST count successfully | ||
| // only attached Routes that may themselves have Accepted: true conditions. | ||
| // Attached Routes whose Accepted condition is False or Unknown (or that | ||
| // do not have an Accepted condition set) MUST NOT be included in this count. |
There was a problem hiding this comment.
I haven't been following this as closely as I should, but maybe I can reword this to make sure I'm understanding it correctly:
AttachedRoutesshould always be set on each Listener, even if the Listener itself has not been acceptedAttachedRoutesshould only represent the number of Routes with theAcceptedcondition set toTruethat have been attached to this Listener
If so, I'm completely on board with point 2, but confused by point 1 (although I recognize that it's not new in this PR).
/cc @rikatz @youngnick
There was a problem hiding this comment.
@robscott Exactly, I changed only point 2 since it is what the issue is requesting, instead for point 1 I can confirm that based on the doc it seems to work in this way.
There was a problem hiding this comment.
hey Rob, so long story short, during the update of ListenerSet GEP (https://github.com/kubernetes-sigs/gateway-api/pull/4286/files) Nick questioned why we were counting Listeners not accepted as well, and well, me and @davidjumani copied the text from here.
In fact, the implementations today count just the accepted ones, but the text on the API description was wrong, so we are now "fixing it" and making it clear that AttachedRoutes MUST count just accepted routes.
Maybe @youngnick has some better wording here tho :)
There was a problem hiding this comment.
@davidesalerno asked me to clarify here, my point is exactly his: we are touching here just what an attachedRoute means (eg.: instead of any attached route, just accepted attached routes) to reflect the same behavior of attachedListenerSets.
We are not touching the line that says "if a Listener on a Gateway has accepted=False it still MUST be added", this behavior should remain the same.
There was a problem hiding this comment.
I think this is fine, since we will have the conformance tests to validate the correct behavior.
k8s-ci-robot
added
release-note
5 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR is fixing the documentation for the AttachedRoutes field specifying that counts only Accepted = true Routes
Which issue(s) this PR fixes:
Fixes #4321
Does this PR introduce a user-facing change?: