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.

Sign up for GitHub

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

Jump to bottom

Fix "list_type_missing" API violations in meta/v1 #121759

Merged
merged 18 commits into from
Dec 13, 2023
Merged

Fix "list_type_missing" API violations in meta/v1 #121759

merged 18 commits into from
Dec 13, 2023

Conversation

thockin
Copy link
Member

@thockin thockin commented Nov 6, 2023 •
edited
Loading

This assumes that any such field is atomic, except:

  • OwnerReferences: because it has a +patchStrategy=merge, but it probably needs a +listMapKey=... ?
  • Finalizers: because it hs a +patchStrategy=merge, but is a primitive type (string).

An alternative approach could be just to turn off the API warnings for these fields, but it felt more correct to declare the semantics.

If this flies I can do others.

@apelisse to tell me if I totally missed the mark on this assumption and to advise on the above.
@liggitt FYI
@deads2k For consult

/kind cleanup

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 6, 2023
@thockin thockin added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. labels Nov 6, 2023
@k8s-ci-robot k8s-ci-robot added area/code-generation kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 6, 2023
liggitt
liggitt reviewed Nov 6, 2023
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/runtime/types.go Outdated
@@ -102,6 +102,7 @@ type RawExtension struct {
// Raw is the underlying serialization of this object.
//
// TODO: Determine how to detect ContentType and ContentEncoding of 'Raw' data.
// +listType=atomic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these look ~fine for now, but we could also fix the linter to not complain about []byte "lists". cc @apelisse

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fair enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kubernetes/kube-openapi#437

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will back these out from this PR, but as a new commit for readability

apelisse
apelisse reviewed Nov 6, 2023
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go Show resolved Hide resolved
staging/src/k8s.io/apimachinery/pkg/runtime/types.go Outdated
@@ -102,6 +102,7 @@ type RawExtension struct {
// Raw is the underlying serialization of this object.
//
// TODO: Determine how to detect ContentType and ContentEncoding of 'Raw' data.
// +listType=atomic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fair enough.

@alexzielenski
Copy link
Contributor

/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 Nov 7, 2023
@thockin thockin mentioned this pull request Nov 11, 2023
Merged
@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Nov 12, 2023
@k8s-ci-robot k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 12, 2023
@thockin thockin added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 12, 2023
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 12, 2023
@thockin thockin changed the title Fix "list_type_missing" API violations in meta/v1 WIP: Fix "list_type_missing" API violations in meta/v1 Nov 12, 2023
@thockin thockin changed the title WIP: Fix "list_type_missing" API violations in meta/v1 Fix "list_type_missing" API violations in meta/v1 Nov 27, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2023
@thockin
Copy link
Member Author

thockin commented Nov 27, 2023

No longer WIP. I removed the config-API commit and we can look at those as a followup

@liggitt
Copy link
Member

liggitt commented Nov 28, 2023

/lgtm
/approve
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 28, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e246d6a9aeb09ef8381af91dca2a0778da23be3f

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, thockin

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2023
@liggitt
Copy link
Member

liggitt commented Nov 28, 2023

(unit test flake is #121926)

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2023
@thockin
Copy link
Member Author

thockin commented Nov 30, 2023

/retest

@thockin thockin removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2023
@bart0sh bart0sh moved this from WIP to Needs Approver in SIG Node PR Triage Dec 8, 2023
@k8s-ci-robot k8s-ci-robot merged commit 8a22571 into kubernetes:master Dec 13, 2023
18 checks passed
SIG Node PR Triage automation moved this from Needs Approver to Done Dec 13, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Dec 13, 2023
@pastequo pastequo mentioned this pull request Jan 25, 2024
Open
@pastequo
Copy link

Should this PR be back-ported in 1.29 and others ? It might be useful for Customer Resources that includes native types into their definition, to have the full granularity for managedfields

https://kubernetes.slack.com/archives/C0123CNN8F3/p1705594643726679

@dprotaso
Copy link
Contributor

Brought this up at sig-api-machinery this week and folks were ok with a cherry-picking this back (notes, video) to a released version of K8s - in order to fix SSA

/cherrypick release-1.29

@liggitt
Copy link
Member

liggitt commented Jan 25, 2024

Brought this up at sig-api-machinery this week and folks were ok with a cherry-picking this back (notes, video) to a released version of K8s - in order to fix SSA

What specifically would this fix in SSA? the only two fields that actually changed anything effectively in this PR were ownerReferences and finalizers, right? neither of those were the ones mentioned in the linked slack thread

Backporting API behavior to a minor doesn't seem like a great idea to me. Letting the change take effect in the next release seems more reasonable.

@dprotaso
Copy link
Contributor

What specifically would this fix in SSA? the only two fields that actually changed anything effectively in this PR were ownerReferences and finalizers, right? neither of those were the ones mentioned in the linked slack thread

Various fields in the PodSpec now have the proper listType and listMapKey annotations - thus for those that embed this type into their own CRDs they'll have the proper OpenAPI schema when generating these CRD definitions.

@dprotaso
Copy link
Contributor

Backporting API behavior to a minor doesn't seem like a great idea to me. Letting the change take effect in the next release seems more reasonable.

Yeah that's fair - seems like the cherry-picker plugin isn't enabled anymore ;)

@liggitt
Copy link
Member

liggitt commented Jan 25, 2024 •
edited
Loading

Various fields in the PodSpec now have the proper listType and listMapKey annotations - thus for those that embed this type into their own CRDs they'll have the proper OpenAPI schema when generating these CRD definitions.

Ah. Letting folks bump to k8s.io/api v0.30.0 when its released would let them regen with annotations, right? I'd probably opt for that.

seems like the cherry-picker plugin isn't enabled anymore ;)

it was never enabled for kubernetes/kubernetes, fyi

@thockin thockin deleted the fix_api_violations branch March 10, 2024 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@apelisse apelisse apelisse left review comments

@deads2k deads2k Awaiting requested review from deads2k

@Jefftree Jefftree Awaiting requested review from Jefftree

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kube-proxy area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Apps
Archived in project
SIG Auth
Archived in project
SIG CLI
Archived in project
SIG Node PR Triage
Archived in project
SIG Node PR Triage
Done
Milestone
v1.30
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@thockin @alexzielenski @liggitt @apelisse @k8s-ci-robot @pastequo @dprotaso