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

update NewObject for groupversionkind #18379

Merged
merged 1 commit into from
Dec 13, 2015
Merged

update NewObject for groupversionkind #18379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 11 additions & 11 deletions pkg/api/copy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,24 +32,24 @@ import (

func TestDeepCopyApiObjects(t *testing.T) {
for i := 0; i < *fuzzIters; i++ {
for _, gv := range []unversioned.GroupVersion{testapi.Default.InternalGroupVersion(), *testapi.Default.GroupVersion()} {
f := apitesting.FuzzerFor(t, gv.String(), rand.NewSource(rand.Int63()))
for kind := range api.Scheme.KnownTypes(gv) {
doDeepCopyTest(t, gv.String(), kind, f)
for _, version := range []unversioned.GroupVersion{testapi.Default.InternalGroupVersion(), *testapi.Default.GroupVersion()} {
f := apitesting.FuzzerFor(t, version.String(), rand.NewSource(rand.Int63()))
Copy link
Member

Choose a reason for hiding this comment

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

I know we've gone through this before and you think we don't need to name the variable groupVersion because we've properly typed it now, but I still think naming it groupVersion is better for readability. For example, when reading this line, now I have to find the definition of version to know what's contained in it.

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 know we've gone through this before and you think we don't need to name the variable groupVersion because we've properly typed it now, but I still think naming it groupVersion is better for readability. For example, when reading this line, now I have to find the definition of version to know what's contained in it.

Doesn't that end up feeling like hungarian notation? I'd say that group is a string, version is a GroupVersion, and kind is a GroupVersionKind. Any variant deserves a different name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@derekwaynecarr I know you were against gv and gvk. Do you have an opinion?

Copy link
Member

Choose a reason for hiding this comment

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

Actually do we have variable version that is a GroupVersion.Version? If not, I'm fine with using version for GroupVersion everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

I think GroupVersionKind type name is basically Hungarian notation since its more prescriptive of the individual values it holds than the use case it fulfills. The contrast in my head is we don't call where your home is a "NumberStreetCityStateZIP", it's an Address.

That said, tuple and triplet naming is awkward, and I am not surprised we went down that path since we are mostly trying to clean up existing code that had them as three separate fields. So I am not against GroupVersion or GroupVersionKind, but if we create a GroupVersionKindField, to describe a field in the API, and start creating gvkf acronyms in the code, we have gone too far.

To me, I would think things as follows:

  • group is a string
  • GroupVersion is more like a Package if I were to take Java naming conventions. This also make sense to me since if we start to support third-party API groups, I would want to add the other fields to a Package like vendor for example.
  • kind is a Kind, which is a fully qualified means of addressing an object in a Package. so kind.Kind is a string, and kind.Package is a Package.
  • field is a Field, which is a fully qualified means of addressing a member in a Kind. so field.Field would be a name we can pass into reflection, etc.

If we are not wanting to describe things in something more akin to the above convention, then I think its confusing to not expand our internal acronyms for folks that come into the code. I prefer clarity so gv becomes groupVersion, gvk becomes groupVersionKind.

Copy link
Member

Choose a reason for hiding this comment

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

  • GroupVersion is more like a Package if I were to take Java naming conventions. This also make sense to me since if we start to support third-party API groups, I would want to add the other fields to a Package like vendor for example.

I think somewhere smarterclayton, lavalamp, and bgrant0607 has agreed vendor should be part of the GroupVersion.Group, and use "." to delimiter vendor and group.

Copy link
Member

Choose a reason for hiding this comment

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

@caesarxuchao - I think what we name each token in a URL path is different than what we call the collection of path segments in the internal API server code base. I still think Package is a more sensible name for what it is than GroupVersion, and GroupVersionKind is an eye-sore, and when I know we end up doing GroupVersionKindField we will all hate ourselves little on the inside ;-)

Copy link
Member

Choose a reason for hiding this comment

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

I think what we name each token in a URL path is different than what we call the collection of path segments in the internal API server code base.

Yes, you are right.

Package sounds too generic. This struct currently lives in package unversioned, so in most places we would refer it as unversioned.Package, which doesn't contain much information. Perhaps something like APIPackage.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, group should be a domain name for disambiguation (mygroup.mycompany.com).
We should probably make explicit that our groups with no domain following
are in the k8s.io domain or some such.

On Wed, Dec 9, 2015 at 11:28 PM, Chao Xu notifications@github.com wrote:

In pkg/api/copy_test.go
#18379 (comment)
:

@@ -32,24 +32,24 @@ import (

func TestDeepCopyApiObjects(t *testing.T) {
for i := 0; i < *fuzzIters; i++ {

  •   for _, gv := range []unversioned.GroupVersion{testapi.Default.InternalGroupVersion(), *testapi.Default.GroupVersion()} {

  •       f := apitesting.FuzzerFor(t, gv.String(), rand.NewSource(rand.Int63()))

  •       for kind := range api.Scheme.KnownTypes(gv) {

  •           doDeepCopyTest(t, gv.String(), kind, f)

  •   for _, version := range []unversioned.GroupVersion{testapi.Default.InternalGroupVersion(), *testapi.Default.GroupVersion()} {

  •       f := apitesting.FuzzerFor(t, version.String(), rand.NewSource(rand.Int63()))
  • GroupVersion is more like a Package if I were to take Java naming
    conventions. This also make sense to me since if we start to support
    third-party API groups, I would want to add the other fields to a
    Package like vendor for example.

I think somewhere smarterclayton, lavalamp, and bgrant0607 has agreed
vendor should be part of the GroupVersion.Group, and use "." to delimiter
vendor and group.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/18379/files#r47190465.

for kind := range api.Scheme.KnownTypes(version) {
doDeepCopyTest(t, version.WithKind(kind), f)
}
}
}
}

func doDeepCopyTest(t *testing.T, version, kind string, f *fuzz.Fuzzer) {
item, err := api.Scheme.New(version, kind)
func doDeepCopyTest(t *testing.T, kind unversioned.GroupVersionKind, f *fuzz.Fuzzer) {
item, err := api.Scheme.New(kind)
if err != nil {
t.Fatalf("Could not create a %s: %s", kind, err)
t.Fatalf("Could not create a %v: %s", kind, err)
}
f.Fuzz(item)
itemCopy, err := api.Scheme.DeepCopy(item)
if err != nil {
t.Errorf("Could not deep copy a %s: %s", kind, err)
t.Errorf("Could not deep copy a %v: %s", kind, err)
return
}

Expand All @@ -60,9 +60,9 @@ func doDeepCopyTest(t *testing.T, version, kind string, f *fuzz.Fuzzer) {

func TestDeepCopySingleType(t *testing.T) {
for i := 0; i < *fuzzIters; i++ {
for _, version := range []string{"", testapi.Default.GroupVersion().String()} {
f := apitesting.FuzzerFor(t, version, rand.NewSource(rand.Int63()))
doDeepCopyTest(t, version, "Pod", f)
for _, version := range []unversioned.GroupVersion{testapi.Default.InternalGroupVersion(), *testapi.Default.GroupVersion()} {
f := apitesting.FuzzerFor(t, version.String(), rand.NewSource(rand.Int63()))
doDeepCopyTest(t, version.WithKind("Pod"), f)
}
}
}
4 changes: 2 additions & 2 deletions pkg/api/serialization_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func TestList(t *testing.T) {
defer api.Scheme.Log(nil)

kind := "List"
item, err := api.Scheme.New("", kind)
item, err := api.Scheme.New(api.SchemeGroupVersion.WithKind(kind))
if err != nil {
t.Errorf("Couldn't make a %v? %v", kind, err)
return
Expand Down Expand Up @@ -172,7 +172,7 @@ func TestRoundTripTypes(t *testing.T) {
}

func doRoundTripTest(kind string, t *testing.T) {
item, err := api.Scheme.New(testapi.Default.InternalGroupVersion().String(), kind)
item, err := api.Scheme.New(testapi.Default.InternalGroupVersion().WithKind(kind))
if err != nil {
t.Fatalf("Couldn't make a %v? %v", kind, err)
}
Expand Down
24 changes: 12 additions & 12 deletions pkg/apiserver/api_installer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,9 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
admit := a.group.Admit
context := a.group.Context

serverGroupVersion := a.group.GroupVersion
if a.group.ServerGroupVersion != nil {
serverGroupVersion = *a.group.ServerGroupVersion
optionsExternalVersion := a.group.GroupVersion
if a.group.OptionsExternalVersion != nil {
optionsExternalVersion = *a.group.OptionsExternalVersion
}

var resource, subresource string
Expand Down Expand Up @@ -148,7 +148,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
}
kind := fqKindToRegister.Kind

versionedPtr, err := a.group.Creater.New(a.group.GroupVersion.String(), kind)
versionedPtr, err := a.group.Creater.New(a.group.GroupVersion.WithKind(kind))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -217,22 +217,22 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
if isLister {
list := lister.NewList()
listGVK, err := a.group.Typer.ObjectKind(list)
versionedListPtr, err := a.group.Creater.New(a.group.GroupVersion.String(), listGVK.Kind)
versionedListPtr, err := a.group.Creater.New(a.group.GroupVersion.WithKind(listGVK.Kind))
if err != nil {
return nil, err
}
versionedList = indirectArbitraryPointer(versionedListPtr)
}

versionedListOptions, err := a.group.Creater.New(serverGroupVersion.String(), "ListOptions")
versionedListOptions, err := a.group.Creater.New(optionsExternalVersion.WithKind("ListOptions"))
if err != nil {
return nil, err
}

var versionedDeleterObject interface{}
switch {
case isGracefulDeleter:
objectPtr, err := a.group.Creater.New(serverGroupVersion.String(), "DeleteOptions")
objectPtr, err := a.group.Creater.New(optionsExternalVersion.WithKind("DeleteOptions"))
if err != nil {
return nil, err
}
Expand All @@ -242,7 +242,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
gracefulDeleter = rest.GracefulDeleteAdapter{Deleter: deleter}
}

versionedStatusPtr, err := a.group.Creater.New(serverGroupVersion.String(), "Status")
versionedStatusPtr, err := a.group.Creater.New(optionsExternalVersion.WithKind("Status"))
if err != nil {
return nil, err
}
Expand All @@ -262,9 +262,9 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
return nil, err
}
// TODO this should be a list of all the different external versions we can coerce into the internalKind
getOptionsExternalKind = serverGroupVersion.WithKind(getOptionsInternalKind.Kind)
getOptionsExternalKind = optionsExternalVersion.WithKind(getOptionsInternalKind.Kind)

versionedGetOptions, err = a.group.Creater.New(serverGroupVersion.String(), getOptionsInternalKind.Kind)
versionedGetOptions, err = a.group.Creater.New(optionsExternalVersion.WithKind(getOptionsInternalKind.Kind))
if err != nil {
return nil, err
}
Expand All @@ -287,9 +287,9 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
return nil, err
}
// TODO this should be a list of all the different external versions we can coerce into the internalKind
connectOptionsExternalKind = serverGroupVersion.WithKind(connectOptionsInternalKind.Kind)
connectOptionsExternalKind = optionsExternalVersion.WithKind(connectOptionsInternalKind.Kind)

versionedConnectOptions, err = a.group.Creater.New(serverGroupVersion.String(), connectOptionsInternalKind.Kind)
versionedConnectOptions, err = a.group.Creater.New(optionsExternalVersion.WithKind(connectOptionsInternalKind.Kind))
}
}

Expand Down
11 changes: 6 additions & 5 deletions pkg/apiserver/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,20 @@ type Mux interface {
type APIGroupVersion struct {
Storage map[string]rest.Storage

Root string
Root string

// GroupVersion is the external group version
GroupVersion unversioned.GroupVersion

// RequestInfoResolver is used to parse URLs for the legacy proxy handler. Don't use this for anything else
// TODO: refactor proxy handler to use sub resources
RequestInfoResolver *RequestInfoResolver

// ServerVersion controls the Kubernetes APIVersion used for common objects in the apiserver
// OptionsExternalVersion controls the Kubernetes APIVersion used for common objects in the apiserver
// schema like api.Status, api.DeleteOptions, and unversioned.ListOptions. Other implementors may
// define a version "v1beta1" but want to use the Kubernetes "v1" internal objects. If
// empty, defaults to Version.
// TODO this seems suspicious. Is this actually just "unversioned" now?
ServerGroupVersion *unversioned.GroupVersion
// empty, defaults to GroupVersion.
OptionsExternalVersion *unversioned.GroupVersion

Mapper meta.RESTMapper

Expand Down
24 changes: 12 additions & 12 deletions pkg/apiserver/apiserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ func handleInternal(storage map[string]rest.Storage, admissionControl admission.
group := template
group.Root = "/" + grouplessPrefix
group.GroupVersion = grouplessGroupVersion
group.ServerGroupVersion = &grouplessGroupVersion
group.OptionsExternalVersion = &grouplessGroupVersion
group.Codec = grouplessCodec
if err := (&group).InstallREST(container); err != nil {
panic(fmt.Sprintf("unable to install container %s: %v", group.GroupVersion, err))
Expand All @@ -249,7 +249,7 @@ func handleInternal(storage map[string]rest.Storage, admissionControl admission.
group := template
group.Root = "/" + prefix
group.GroupVersion = testGroupVersion
group.ServerGroupVersion = &testGroupVersion
group.OptionsExternalVersion = &testGroupVersion
group.Codec = codec
if err := (&group).InstallREST(container); err != nil {
panic(fmt.Sprintf("unable to install container %s: %v", group.GroupVersion, err))
Expand All @@ -261,7 +261,7 @@ func handleInternal(storage map[string]rest.Storage, admissionControl admission.
group := template
group.Root = "/" + prefix
group.GroupVersion = newGroupVersion
group.ServerGroupVersion = &newGroupVersion
group.OptionsExternalVersion = &newGroupVersion
group.Codec = newCodec
if err := (&group).InstallREST(container); err != nil {
panic(fmt.Sprintf("unable to install container %s: %v", group.GroupVersion, err))
Expand Down Expand Up @@ -2244,9 +2244,9 @@ func TestUpdateREST(t *testing.T) {
Context: requestContextMapper,
Mapper: namespaceMapper,

GroupVersion: newGroupVersion,
ServerGroupVersion: &newGroupVersion,
Codec: newCodec,
GroupVersion: newGroupVersion,
OptionsExternalVersion: &newGroupVersion,
Codec: newCodec,
}
}

Expand Down Expand Up @@ -2326,9 +2326,9 @@ func TestParentResourceIsRequired(t *testing.T) {
Context: requestContextMapper,
Mapper: namespaceMapper,

GroupVersion: newGroupVersion,
ServerGroupVersion: &newGroupVersion,
Codec: newCodec,
GroupVersion: newGroupVersion,
OptionsExternalVersion: &newGroupVersion,
Codec: newCodec,
}
container := restful.NewContainer()
if err := group.InstallREST(container); err == nil {
Expand All @@ -2355,9 +2355,9 @@ func TestParentResourceIsRequired(t *testing.T) {
Context: requestContextMapper,
Mapper: namespaceMapper,

GroupVersion: newGroupVersion,
ServerGroupVersion: &newGroupVersion,
Codec: newCodec,
GroupVersion: newGroupVersion,
OptionsExternalVersion: &newGroupVersion,
Codec: newCodec,
}
container = restful.NewContainer()
if err := group.InstallREST(container); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/apiserver/resthandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func getRequestOptions(req *restful.Request, scope RequestScope, internalKind, e
query = newQuery
}

versioned, err := scope.Creater.New(externalKind.GroupVersion().String(), externalKind.Kind)
versioned, err := scope.Creater.New(externalKind)
if err != nil {
return nil, err
}
Expand Down
24 changes: 12 additions & 12 deletions pkg/client/unversioned/testclient/fixture.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,24 +152,24 @@ func NewObjects(scheme ObjectScheme, decoder runtime.ObjectDecoder) ObjectRetrie
}
}

func (o objects) Kind(gvk unversioned.GroupVersionKind, name string) (runtime.Object, error) {
func (o objects) Kind(kind unversioned.GroupVersionKind, name string) (runtime.Object, error) {
// TODO our test clients deal in internal versions. We need to plumb that knowledge down here
// we might do this via an extra function to the scheme to allow getting internal group versions
// I'm punting for now
gvk.Version = ""
kind.Version = ""

empty, _ := o.scheme.New(gvk.GroupVersion().String(), gvk.Kind)
empty, _ := o.scheme.New(kind)
nilValue := reflect.Zero(reflect.TypeOf(empty)).Interface().(runtime.Object)

arr, ok := o.types[gvk.Kind]
arr, ok := o.types[kind.Kind]
if !ok {
if strings.HasSuffix(gvk.Kind, "List") {
itemKind := gvk.Kind[:len(gvk.Kind)-4]
if strings.HasSuffix(kind.Kind, "List") {
itemKind := kind.Kind[:len(kind.Kind)-4]
arr, ok := o.types[itemKind]
if !ok {
return empty, nil
}
out, err := o.scheme.New(gvk.GroupVersion().String(), gvk.Kind)
out, err := o.scheme.New(kind)
if err != nil {
return nilValue, err
}
Expand All @@ -181,25 +181,25 @@ func (o objects) Kind(gvk unversioned.GroupVersionKind, name string) (runtime.Ob
}
return out, nil
}
return nilValue, errors.NewNotFound(gvk.Kind, name)
return nilValue, errors.NewNotFound(kind.Kind, name)
}

index := o.last[gvk.Kind]
index := o.last[kind.Kind]
if index >= len(arr) {
index = len(arr) - 1
}
if index < 0 {
return nilValue, errors.NewNotFound(gvk.Kind, name)
return nilValue, errors.NewNotFound(kind.Kind, name)
}
out, err := o.scheme.Copy(arr[index])
if err != nil {
return nilValue, err
}
o.last[gvk.Kind] = index + 1
o.last[kind.Kind] = index + 1

if status, ok := out.(*unversioned.Status); ok {
if status.Details != nil {
status.Details.Kind = gvk.Kind
status.Details.Kind = kind.Kind
}
if status.Status != unversioned.StatusSuccess {
return nilValue, &errors.StatusError{ErrStatus: *status}
Expand Down