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

chore(apimachinery): deprecate meta.UnsafeGuessKindToResource #119924

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+123 −273
Open

chore(apimachinery): deprecate meta.UnsafeGuessKindToResource #119924

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e13205
chore(apimachinery): deprecate meta.UnsafeGuessKindToResource
pmalek Aug 13, 2023
333b97d
review: fix broken tests
pmalek Aug 14, 2023
4d1cf44
tests: revert change to TestKindToResource, split it into subtests, a…
pmalek Aug 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ require (
k8s.io/csi-translation-lib v0.0.0
k8s.io/dynamic-resource-allocation v0.0.0
k8s.io/endpointslice v0.0.0
k8s.io/gengo v0.0.0-20220902162205-c0856e24416d
k8s.io/gengo v0.0.0-20230306165830-ab3349d207d4
k8s.io/klog/v2 v2.100.1
k8s.io/kms v0.0.0
k8s.io/kube-aggregator v0.0.0
Expand Down
143 changes: 2 additions & 141 deletions go.sum
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions staging/src/k8s.io/apimachinery/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ require (
google.golang.org/protobuf v1.30.0 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
k8s.io/gengo v0.0.0-20230306165830-ab3349d207d4
)

replace k8s.io/apimachinery => ../apimachinery
22 changes: 10 additions & 12 deletions staging/src/k8s.io/apimachinery/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 13 additions & 4 deletions staging/src/k8s.io/apimachinery/pkg/api/meta/restmapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/gengo/namer"
Copy link
Member

Choose a reason for hiding this comment

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

gengo is for code-generation prior to build, not something we want to rely on at runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. What suggestion do you have on this one? namer ability to provide plural names was suggested in #110053 (comment) as a replacement for UnsafeGuessKindToResource. What other options do we have?

"k8s.io/gengo/types"
)

// Implements RESTScope interface
Expand Down Expand Up @@ -62,6 +64,8 @@ type DefaultRESTMapper struct {
kindToScope map[schema.GroupVersionKind]RESTScope
singularToPlural map[schema.GroupVersionResource]schema.GroupVersionResource
pluralToSingular map[schema.GroupVersionResource]schema.GroupVersionResource

namer namer.Namer
Copy link
Contributor

Choose a reason for hiding this comment

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

may give fmt error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK. What's the fmt tool to check this and how can I trigger it? gofmt doesn't report anything

}

func (m *DefaultRESTMapper) String() string {
Expand Down Expand Up @@ -93,11 +97,15 @@ func NewDefaultRESTMapper(defaultGroupVersions []schema.GroupVersion) *DefaultRE
defaultGroupVersions: defaultGroupVersions,
singularToPlural: singularToPlural,
pluralToSingular: pluralToSingular,
namer: namer.NewAllLowercasePluralNamer(nil),
Copy link
Member

Choose a reason for hiding this comment

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

I don't really see how embedding a namer whose exceptions can't be overridden is an improvement on the current code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pending response on the alternative for namer one question pops in my head: what improvement would you possibly see here? Make NewDefaultRESTMapper a variadic function accepting exceptions or add second "constructor" which allows to specify the exceptions keeping this intact (as is now)?

}
}

func (m *DefaultRESTMapper) Add(kind schema.GroupVersionKind, scope RESTScope) {
plural, singular := UnsafeGuessKindToResource(kind)
t := types.Ref(kind.GroupVersion().String(), kind.Kind)
plural := kind.GroupVersion().WithResource(m.namer.Name(t))
singular := kind.GroupVersion().WithResource(strings.ToLower(kind.Kind))

m.AddSpecific(kind, plural, singular, scope)
}

Expand All @@ -123,6 +131,9 @@ var unpluralizedSuffixes = []string{
// UnsafeGuessKindToResource converts Kind to a resource name.
// Broken. This method only "sort of" works when used outside of this package. It assumes that Kinds and Resources match
// and they aren't guaranteed to do so.
//
// Deprecated: This is and may yield unwanted and unexpected results.
// Use k8s.io/gengo/namer's NewAllLowercasePluralNamer() instead.
Copy link
Member

Choose a reason for hiding this comment

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

same comment about not depending on gengo at runtime

func UnsafeGuessKindToResource(kind schema.GroupVersionKind) ( /*plural*/ schema.GroupVersionResource /*singular*/, schema.GroupVersionResource) {
kindName := kind.Kind
if len(kindName) == 0 {
Expand Down Expand Up @@ -244,7 +255,6 @@ func (m *DefaultRESTMapper) ResourcesFor(input schema.GroupVersionResource) ([]s
ret = append(ret, plural)
}
}

}

case hasVersion:
Expand Down Expand Up @@ -329,7 +339,6 @@ func (m *DefaultRESTMapper) KindsFor(input schema.GroupVersionResource) ([]schem
ret = append(ret, currKind)
}
}

}

case hasVersion:
Expand Down Expand Up @@ -495,7 +504,7 @@ func (m *DefaultRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string
}

for _, gvk := range potentialGVK {
//Ensure we have a REST mapping
// Ensure we have a REST mapping
res, ok := m.kindToPluralResource[gvk]
if !ok {
continue
Expand Down
33 changes: 23 additions & 10 deletions staging/src/k8s.io/apimachinery/pkg/api/meta/restmapper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"testing"

"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/gengo/namer"
"k8s.io/gengo/types"
)

func TestRESTMapperVersionAndKindForResource(t *testing.T) {
Expand Down Expand Up @@ -249,12 +251,10 @@ func TestRESTMapperKindsFor(t *testing.T) {
continue
}
}

} else {
if testCase.ExpectedKinds[0] != singleKind {
t.Errorf("%s: expected %v, got %v", tcName, testCase.ExpectedKinds[0], singleKind)
}

}
}
}
Expand Down Expand Up @@ -426,12 +426,10 @@ func TestRESTMapperResourcesFor(t *testing.T) {
continue
}
}

} else {
if testCase.ExpectedResources[0] != singleResource {
t.Errorf("%s: expected %v, got %v", tcName, testCase.ExpectedResources[0], singleResource)
}

}
}
}
Expand All @@ -448,18 +446,33 @@ func TestKindToResource(t *testing.T) {

// Add "ies" when ending with "y"
{Kind: "ImageRepository", Plural: "imagerepositories", Singular: "imagerepository"},
// But take into account exceptions like gateway.
{Kind: "Gateway", Plural: "gateways", Singular: "gateway"},
// Add "es" when ending with "s"
{Kind: "miss", Plural: "misses", Singular: "miss"},
// Add "s" otherwise
{Kind: "lowercase", Plural: "lowercases", Singular: "lowercase"},
// Take into account Endpoints exception.
{Kind: "Endpoints", Plural: "endpoints", Singular: "endpoints"},
}
for i, testCase := range testCases {
version := schema.GroupVersion{}

plural, singular := UnsafeGuessKindToResource(version.WithKind(testCase.Kind))
Copy link
Member

Choose a reason for hiding this comment

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

revert the changes to this test... this is exercising UnsafeGuessKindToResource which we don't want to lose coverage of

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, reverted this but I retained the change using the subtests and adding the Endpoints test case.

if singular != version.WithResource(testCase.Singular) || plural != version.WithResource(testCase.Plural) {
t.Errorf("%d: unexpected plural and singular: %v %v", i, plural, singular)
}
namer := namer.NewAllLowercasePluralNamer(map[string]string{
"Endpoints": "endpoints",
})
for _, testCase := range testCases {
testCase := testCase
t.Run(testCase.Kind, func(t *testing.T) {
version := schema.GroupVersion{}
gvk := version.WithKind(testCase.Kind)

name := namer.Name(types.Ref(gvk.GroupKind().String(), gvk.Kind))
plural := gvk.GroupVersion().WithResource(name)
singular := gvk.GroupVersion().WithResource(strings.ToLower(gvk.Kind))

if singular != version.WithResource(testCase.Singular) || plural != version.WithResource(testCase.Plural) {
t.Errorf("unexpected plural and singular: %v %v", plural, singular)
}
})
}
}

Expand Down
8 changes: 7 additions & 1 deletion staging/src/k8s.io/client-go/dynamic/fake/simple.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import (
"k8s.io/apimachinery/pkg/watch"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/testing"
"k8s.io/gengo/namer"
gengotypes "k8s.io/gengo/types"
)

func NewSimpleDynamicClient(scheme *runtime.Scheme, objects ...runtime.Object) *FakeDynamicClient {
Expand Down Expand Up @@ -78,12 +80,16 @@ func NewSimpleDynamicClientWithCustomListKinds(scheme *runtime.Scheme, gvrToList
// first we attempt to invert known List types from the scheme to auto guess the resource with unsafe guesses
// this covers common usage of registering types in scheme and passing them
completeGVRToListKind := map[schema.GroupVersionResource]string{}
namer := namer.NewAllLowercasePluralNamer(nil)
for listGVK := range scheme.AllKnownTypes() {
if !strings.HasSuffix(listGVK.Kind, "List") {
continue
}

nonListGVK := listGVK.GroupVersion().WithKind(listGVK.Kind[:len(listGVK.Kind)-4])
plural, _ := meta.UnsafeGuessKindToResource(nonListGVK)
t := gengotypes.Ref(nonListGVK.GroupVersion().String(), nonListGVK.Kind)
plural := nonListGVK.GroupVersion().WithResource(namer.Name(t))

completeGVRToListKind[plural] = listGVK.Kind
}

Expand Down
1 change: 1 addition & 0 deletions staging/src/k8s.io/client-go/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ require (
google.golang.org/protobuf v1.30.0
k8s.io/api v0.0.0
k8s.io/apimachinery v0.0.0
k8s.io/gengo v0.0.0-20230306165830-ab3349d207d4
k8s.io/klog/v2 v2.100.1
k8s.io/kube-openapi v0.0.0-20230717233707-2695361300d9
k8s.io/utils v0.0.0-20230406110748-d93618cff8a2
Expand Down