KEP 1645: updates ServiceExport conditions #5437
Conversation
ServiceExport conditions in the KEP are not reflecting what's actually done in the CRD as this predates the addition of the Condition type in Kubernetes. This commit essentially put back all the condition types that we actually have in MCS-API code and change the reference in the example (the regular kubernetes type requires a message to be added!) Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
kind/kep
| // Users should not expect detailed per-cluster information in the | ||
| // conflict message. | ||
| ServiceExportConflict = "Conflict" | ||
| ) |
There was a problem hiding this comment.
Perhaps we should just link to the Go code in the mcs-api repo rather than copying here and having to keep it up to date?
There was a problem hiding this comment.
Hmm I don't think that works very well because when we are doing the opposite meaning modifying the KEP to then update the code there it is significantly more clear what you want to actually achieve to update this inline I think
There was a problem hiding this comment.
Apparently we haven't been doing that workflow
There was a problem hiding this comment.
AFAIK for new fields and things like that we have been doing this workflow mostly, and it should probably have been done for this condition change a while ago too.
I don't think that it's super important that this go code inline match absolutely 1:1 what we have in the mcs-api repo, it's not a big deal if small details drift. This one isn't that small though and the most important part is that I personally got super confused and had to debug this for longer than necessary when I did the Cilium implementation because I had to discover that the message field was required now so at the very least the yaml update below solves that...
There was a problem hiding this comment.
OK.
BTW: while the message field is required in the CRD def, it can be empty. I was wondering about that too initially. When using the Go structs, Message is a non-pointer so is rendered as message: "" if not specified in code. However reason cannot be empty as it has a regex pattern which disallows empty.
There was a problem hiding this comment.
Argh indeed I forgot about Reason vs Message I definitely want to update that to not bamboozle future implementers :/
There was a problem hiding this comment.
here is the PR fixing that: #5437
There was a problem hiding this comment.
If I am copy pasting this correctly it would work better :/: #5438
|
I don’t think we can avoid having a duplicate. /approve Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MrFreezeex, skitt 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 |
k8s-ci-robot
added
the
approved
|
/lgtm |
k8s-ci-robot
added
the
lgtm
ServiceExport conditions in the KEP are not reflecting what's actually done in the CRD as this predates the addition of the Condition type in Kubernetes.
This commit essentially put back all the condition types that we actually have in MCS-API code and change the reference in the example (the regular kubernetes type requires a message to be added!)