Fix OpenAPI validations by adding API list markers #3964
Conversation
k8s-ci-robot
added
release-note
erikgb
force-pushed
the
fix-openapi-violations
branch
from
501acc1 to
a716675
Compare
|
hey @erikgb As we are moving with the enablement of automated API validations, can you please check #3917 and https://docs.google.com/document/d/1DSyP7Vt9E9wX2ku1C0AvCLqQe9cCiDPCLiPP0wXFYQw/edit?disco=AAABnQWiOX8 and maybe add https://github.com/kubernetes-sigs/kube-api-linter/blob/main/pkg/analysis/ssatags/ to the analyzers list to validate the behavior during CI/CD? Regarding this being a breaking change or not, per Joel's comment on the document we need to check with API Machinery if this change can represent any harm for already implemented or in use Gateway API resources. Thanks! |
Cool! I didn't know that you were in the process of adding KAL to Gateway API. I will definitely take a look and see if we can validate more in CI/CD.
Understood! I am a bit curious myself, as we are in the process of adding missing API list markers to cert-manager APIs. So, in general, it would be good to know which changes can be made non-breaking. Of course, the safest would be to mark all lists as |
erikgb
force-pushed
the
fix-openapi-violations
branch
from
a716675 to
1c16684
Compare
thank you! About your questions on what is safer or not, I need to defer to people who actually know what they are doing (which is not me!) @youngnick @robscott do you see any issue here? On a previous review on the linter proposal document, @JoelSpeed suggested that we keep the @erikgb once you have some answer from apimachinery as well, let's see how this fits on the proposal. Thank you! |
apis/v1/grpcroute_types.go
Outdated
| @@ -137,12 +137,14 @@ type GRPCRouteSpec struct { | |||
| // Support: Core | |||
| // | |||
| // +optional | |||
| // +listType=set | |||
There was a problem hiding this comment.
Pre-existing validation that ensures that these are unique?
There was a problem hiding this comment.
I cannot find such validation. Is this a strict requirement? Probably yes, as the api-server will reject a request with duplicate values (if changing this to set)?
There was a problem hiding this comment.
I believe this could be breaking otherwise yes. Unless we can prove that the CRD ratcheting validation would cover this change?
There was a problem hiding this comment.
Even if the change is technically breaking, I think the key questions are:
- Does it make sense with duplicate values in this list?
- Does the order of the values in the list matter?
- Must the list be owned by a single SSA field manager?
If the answer to all the questions is NO, then I think it could be justified to "break" this as a bug fix.
There was a problem hiding this comment.
I agree that this could/should be considered a bug fix. To keep things clean, I'd recommend starting with +listType=atomic in this PR to match the current and unfortunately flawed spec today, and then file a follow up PR to fix this after this initial PR has merged. That will make it easier to point our release notes to a single small PR for the breaking changes that swap atomic to set.
There was a problem hiding this comment.
Sounds like a good approach. I will update this PR.
There was a problem hiding this comment.
@robscott I have now updated the PR, setting all list types to atomic for now, and prepared another branch with my proposed "breaking" changes. PTAL!
There was a problem hiding this comment.
And here is the WIP PR that I will rebase after this PR is merged: #3972
k8s-ci-robot
added
the
needs-rebase
erikgb
force-pushed
the
fix-openapi-violations
branch
from
1c16684 to
b42ad96
Compare
k8s-ci-robot
removed
the
needs-rebase
erikgb
force-pushed
the
fix-openapi-violations
branch
from
b42ad96 to
8d6109b
Compare
erikgb
force-pushed
the
fix-openapi-violations
branch
from
8d6109b to
30733d6
Compare
erikgb
force-pushed
the
fix-openapi-violations
branch
from
30733d6 to
4096a1d
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
Thanks @erikgb! /lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: erikgb, youngnick 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
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Our openapi generator also generates a report of violations of the Kubernetes API best practices. This report is "thrown away" now, but this PR suggests adding it to Git to raise awareness of the importance of API array metadata.
I have also tried to fix all reported violations in our APIs, but please review carefully!UPDATE: For now, all new list type markers are set toatomic(default without the marker), and I have created a WIP PR (#3972) for the proposed bugfixes that we can consider after merging this.I have also reorganized the markers on some of the affected fields, as I think it's clearer to have a consistent order of the markers.
Which issue(s) this PR fixes:
Fixes #2942
Does this PR introduce a user-facing change?: