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

use GroupVersion in APIGroupVersion for api installer #17175

Merged
merged 1 commit into from
Nov 13, 2015
Merged

use GroupVersion in APIGroupVersion for api installer #17175

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
9 changes: 9 additions & 0 deletions pkg/api/unversioned/group_version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,15 @@ func ParseGroupVersion(gv string) (GroupVersion, error) {
}
}

func ParseGroupVersionOrDie(gv string) GroupVersion {
Copy link
Member

Choose a reason for hiding this comment

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

This is also the first function I added.

ret, err := ParseGroupVersion(gv)
if err != nil {
panic(err)
}

return ret
}

// MarshalJSON implements the json.Marshaller interface.
func (gv GroupVersion) MarshalJSON() ([]byte, error) {
s := gv.String()
Expand Down
30 changes: 15 additions & 15 deletions pkg/apiserver/api_installer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (a *APIInstaller) NewWebService() *restful.WebService {
// TODO: change to restful.MIME_JSON when we set content type in client
ws.Consumes("*/*")
ws.Produces(restful.MIME_JSON)
ws.ApiVersion(a.group.Version)
ws.ApiVersion(a.group.GroupVersion.String())

return ws
}
Expand All @@ -104,9 +104,9 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
admit := a.group.Admit
context := a.group.Context

serverVersion := a.group.ServerVersion
if len(serverVersion) == 0 {
serverVersion = a.group.Version
serverGroupVersion := a.group.GroupVersion
if a.group.ServerGroupVersion != nil {
serverGroupVersion = *a.group.ServerGroupVersion
}

var resource, subresource string
Expand All @@ -126,13 +126,13 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
if err != nil {
return nil, err
}
versionedPtr, err := a.group.Creater.New(a.group.Version, kind)
versionedPtr, err := a.group.Creater.New(a.group.GroupVersion.String(), kind)
if err != nil {
return nil, err
}
versionedObject := indirectArbitraryPointer(versionedPtr)

mapping, err := a.group.Mapper.RESTMapping(kind, a.group.Version)
mapping, err := a.group.Mapper.RESTMapping(kind, a.group.GroupVersion.String())
if err != nil {
return nil, err
}
Expand All @@ -148,7 +148,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
if err != nil {
return nil, err
}
parentMapping, err := a.group.Mapper.RESTMapping(parentKind, a.group.Version)
parentMapping, err := a.group.Mapper.RESTMapping(parentKind, a.group.GroupVersion.String())
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to change the signature of RESTMapping to use GroupVersion instead of string?

Copy link
Member

Choose a reason for hiding this comment

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

one step at a time. I vastly prefer a PR that addresses one package at a time for a sweeping change like this

Copy link
Member

Choose a reason for hiding this comment

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

I mean do you plan to do that in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to change the signature of RESTMapping to use GroupVersion instead of string?

I've just started by adjusting RESTMapping to use a GroupVersionKind and rippling that out instead of APIVersion and Kind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to change the signature of RESTMapping to use GroupVersion instead of string?

Also, yes, that's on my list. I'm trying to keep these pieces small though..

if err != nil {
return nil, err
}
Expand Down Expand Up @@ -181,22 +181,22 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
if isLister {
list := lister.NewList()
_, listKind, err := a.group.Typer.ObjectVersionAndKind(list)
versionedListPtr, err := a.group.Creater.New(a.group.Version, listKind)
versionedListPtr, err := a.group.Creater.New(a.group.GroupVersion.String(), listKind)
if err != nil {
return nil, err
}
versionedList = indirectArbitraryPointer(versionedListPtr)
}

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

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

versionedStatusPtr, err := a.group.Creater.New(serverVersion, "Status")
versionedStatusPtr, err := a.group.Creater.New(serverGroupVersion.String(), "Status")
if err != nil {
return nil, err
}
Expand All @@ -224,7 +224,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
if err != nil {
return nil, err
}
versionedGetOptions, err = a.group.Creater.New(serverVersion, getOptionsKind)
versionedGetOptions, err = a.group.Creater.New(serverGroupVersion.String(), getOptionsKind)
if err != nil {
return nil, err
}
Expand All @@ -245,7 +245,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
if err != nil {
return nil, err
}
versionedConnectOptions, err = a.group.Creater.New(serverVersion, connectOptionsKind)
versionedConnectOptions, err = a.group.Creater.New(serverGroupVersion.String(), connectOptionsKind)
}
}

Expand Down Expand Up @@ -379,8 +379,8 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
Creater: a.group.Creater,
Convertor: a.group.Convertor,
Codec: mapping.Codec,
APIVersion: a.group.Version,
ServerAPIVersion: serverVersion,
APIVersion: a.group.GroupVersion.String(),
ServerAPIVersion: serverGroupVersion.String(),
Resource: resource,
Subresource: subresource,
Kind: kind,
Expand Down
24 changes: 10 additions & 14 deletions pkg/apiserver/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,8 @@ type Mux interface {
type APIGroupVersion struct {
Storage map[string]rest.Storage

Root string
// TODO: caesarxuchao: Version actually contains "group/version", refactor it to avoid confusion.
Version string
Root string
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
Expand All @@ -91,9 +90,8 @@ type APIGroupVersion struct {
// schema like api.Status, api.DeleteOptions, and api.ListOptions. Other implementors may
// define a version "v1beta1" but want to use the Kubernetes "v1" internal objects. If
// empty, defaults to Version.
// TODO: caesarxuchao: ServerVersion actually contains "group/version",
// refactor it to avoid confusion.
ServerVersion string
// TODO this seems suspicious. Is this actually just "unversioned" now?
ServerGroupVersion *unversioned.GroupVersion
Copy link
Member

Choose a reason for hiding this comment

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

v1 has to keep returning "v1" for those unversioned objects, not sure what I would expect from other group/versions

Copy link
Member

Choose a reason for hiding this comment

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

Why is this one a pointer? I think it should be of the same type as GroupVersion on line 83 for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

@liggitt, ServerGroupVersion is the versioned used by encode DeleteOptions ListOptions etc. We may not need it anymore since we are moving these things to unversioned.

Copy link
Member

Choose a reason for hiding this comment

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

a nil check on a pointer is equivalent to a len(0) check on a string if we're trying to check for "unsettedness" (though I would sort of expect newer api versions to be returning unversioned status, which is an apiVersion="", right? Is that expressable?)

Copy link
Member

Choose a reason for hiding this comment

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

@liggitt, ServerGroupVersion is the versioned used by encode DeleteOptions ListOptions etc. We may not need it anymore since we are moving these things to unversioned.

I would expect to need it (or something similar) to keep v1 returning "v1" instead of "" for unversioned object types, but I haven't swept how this is used to be sure.

Copy link
Member

Choose a reason for hiding this comment

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

can you spawn a separate issue to look at what is being set for apiVersion on unversioned resources from extensions/v1beta1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you spawn a separate issue to look at what is being set for apiVersion on unversioned resources from extensions/v1beta1?

Done: #17218


Mapper meta.RESTMapper

Expand Down Expand Up @@ -126,8 +124,7 @@ func (g *APIGroupVersion) InstallREST(container *restful.Container) error {
installer := g.newInstaller()
ws := installer.NewWebService()
apiResources, registrationErrors := installer.Install(ws)
// TODO: g.Version only contains "version" now, it will contain "group/version" in the near future.
AddSupportedResourcesWebService(ws, g.Version, apiResources)
AddSupportedResourcesWebService(ws, g.GroupVersion, apiResources)
container.Add(ws)
return utilerrors.NewAggregate(registrationErrors)
}
Expand All @@ -151,14 +148,13 @@ func (g *APIGroupVersion) UpdateREST(container *restful.Container) error {
return apierrors.NewInternalError(fmt.Errorf("unable to find an existing webservice for prefix %s", installer.prefix))
}
apiResources, registrationErrors := installer.Install(ws)
// TODO: g.Version only contains "version" now, it will contain "group/version" in the near future.
AddSupportedResourcesWebService(ws, g.Version, apiResources)
AddSupportedResourcesWebService(ws, g.GroupVersion, apiResources)
return utilerrors.NewAggregate(registrationErrors)
}

// newInstaller is a helper to create the installer. Used by InstallREST and UpdateREST.
func (g *APIGroupVersion) newInstaller() *APIInstaller {
prefix := path.Join(g.Root, g.Version)
prefix := path.Join(g.Root, g.GroupVersion.Group, g.GroupVersion.Version)
installer := &APIInstaller{
group: g,
info: g.RequestInfoResolver,
Expand Down Expand Up @@ -287,7 +283,7 @@ func AddGroupWebService(container *restful.Container, path string, group unversi

// Adds a service to return the supported resources, E.g., a such web service
// will be registered at /apis/extensions/v1.
func AddSupportedResourcesWebService(ws *restful.WebService, groupVersion string, apiResources []unversioned.APIResource) {
func AddSupportedResourcesWebService(ws *restful.WebService, groupVersion unversioned.GroupVersion, apiResources []unversioned.APIResource) {
resourceHandler := SupportedResourcesHandler(groupVersion, apiResources)
ws.Route(ws.GET("/").To(resourceHandler).
Doc("get available resources").
Expand Down Expand Up @@ -328,10 +324,10 @@ func GroupHandler(group unversioned.APIGroup) restful.RouteFunction {
}

// SupportedResourcesHandler returns a handler which will list the provided resources as available.
func SupportedResourcesHandler(groupVersion string, apiResources []unversioned.APIResource) restful.RouteFunction {
func SupportedResourcesHandler(groupVersion unversioned.GroupVersion, apiResources []unversioned.APIResource) restful.RouteFunction {
return func(req *restful.Request, resp *restful.Response) {
// TODO: use restful's Response methods
writeJSON(http.StatusOK, api.Codec, &unversioned.APIResourceList{GroupVersion: groupVersion, APIResources: apiResources}, resp.ResponseWriter, true)
writeJSON(http.StatusOK, api.Codec, &unversioned.APIResourceList{GroupVersion: groupVersion.String(), APIResources: apiResources}, resp.ResponseWriter, true)
}
}

Expand Down