Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Current v1beta1 API Types do not define list-type annotation #6504

Open
Tracked by #10852
chrischdi opened this issue May 12, 2022 · 9 comments
Open
Tracked by #10852

Current v1beta1 API Types do not define list-type annotation #6504

chrischdi opened this issue May 12, 2022 · 9 comments
Labels
area/api Issues or PRs related to the APIs help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@chrischdi
Copy link
Member

What steps did you take and what happened:

While implementing #6462 I stumbled across the warning that we do not define list-type for slices in our v1beta1 structs:

API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassPatch,Definitions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassSpec,Patches
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassSpec,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,JSONSchemaProps,Enum
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,JSONSchemaProps,Required
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineDeploymentVariables,Overrides
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckClass,UnhealthyConditions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckSpec,UnhealthyConditions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckStatus,Targets
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,NetworkRanges,CIDRBlocks
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,PatchDefinition,JSONPatches
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,PatchSelectorMatchMachineDeploymentClass,Names
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,Topology,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersClass,MachineDeployments
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersTopology,MachineDeployments 

This should be the same also for occurencies in v1alpha3 and v1alpha4.

What did you expect to happen:

API rule violation warning does not occur

Anything else you would like to add:

Adding the list-type helps to improve support for server-side apply and allows to define the merge-strategy for server-side apply.

Especially when continuing at #6320 results in making use of server-side apply (as currently proposed) works out adding the list-type to the defintions help improving support here.

Setting the list-type may be considered as a breaking change and may require a new api version.

Environment:

  • Cluster-api version:
  • Minikube/KIND version:
  • Kubernetes version: (use kubectl version):
  • OS (e.g. from /etc/os-release):

/kind bug
/area api

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. area/api Issues or PRs related to the APIs labels May 12, 2022
@sbueringer
Copy link
Member

sbueringer commented May 12, 2022

Sounds good to me,
I'm generally +1 to follow the best practice to define the list type.

I personally wouldn't consider this a breaking change. I think depending on how we set it, it could make the merge behavior of kubectl more precise, but I think that's rather a bugfix.

@fabriziopandini
Copy link
Member

+1
this goes in the direction of @JoelSpeed suggestions about API conventions
/milestone v1.2

@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone May 13, 2022
@JoelSpeed
Copy link
Contributor

+1

Today, because lists are atomic by default, clients will have to observe the entire list, make their updates, and patch the entire list. By adding these annotations, we won't break the existing usage as they will still allow sending the entire list.

Do we know if any of our controllers are already attempting to use server side apply? The only break I could think of is if any controllers are already using SSA, they will go from owning the list to owning all the items in the list. Others would have to use force to override that. If there is already a controller doing this, I would expect this to have already been a problem though.

This assumes of course that the manager name differs, which I know we haven't resolved yet.

In all though, it should allow the controllers to simplify their logic once they do make the move to SSA

@fabriziopandini fabriziopandini added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 29, 2022
@fabriziopandini fabriziopandini removed this from the v1.2 milestone Jul 29, 2022
@fabriziopandini fabriziopandini removed the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 29, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 27, 2022
@fabriziopandini
Copy link
Member

/triage accepted
/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 2, 2022
@fabriziopandini
Copy link
Member

/remove-kind bug
/kind api-change

@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed kind/bug Categorizes issue or PR as related to a bug. labels Nov 2, 2022
@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Jan 19, 2024
@sbueringer
Copy link
Member

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 19, 2024
@fabriziopandini
Copy link
Member

/priority important-longterm

@k8s-ci-robot k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Apr 12, 2024
@fabriziopandini fabriziopandini removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Apr 16, 2024
@fabriziopandini fabriziopandini added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Issues or PRs related to the APIs help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

6 participants