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

API Server - pass path name in context of create request for subresource #7718

Merged
merged 1 commit into from May 6, 2015

Conversation

Projects
None yet
4 participants
@csrwng
Contributor

csrwng commented May 4, 2015

Allows a REST storage for a subresource to obtain name in path from
request.

@googlebot googlebot added the cla: yes label May 4, 2015

@csrwng

This comment has been minimized.

Show comment
Hide comment
@csrwng
Contributor

csrwng commented May 4, 2015

Show outdated Hide outdated pkg/apiserver/resthandler.go
ctx := scope.ContextFunc(req)
ctx = api.WithNamespace(ctx, namespace)
if len(name) > 0 {
ctx = api.WithValue(ctx, "name", name)

This comment has been minimized.

@smarterclayton

smarterclayton May 4, 2015

Contributor

I'm a bit concerned about this being an abuse of contexts. I think I would prefer if we set it onto the object.

@smarterclayton

smarterclayton May 4, 2015

Contributor

I'm a bit concerned about this being an abuse of contexts. I think I would prefer if we set it onto the object.

This comment has been minimized.

@smarterclayton

smarterclayton May 4, 2015

Contributor

For instance, an interface that set the value onto the object that someone could implement.

@smarterclayton

smarterclayton May 4, 2015

Contributor

For instance, an interface that set the value onto the object that someone could implement.

This comment has been minimized.

@liggitt

liggitt May 4, 2015

Member

I thought we were not supposed to add functions to API objects

@liggitt

liggitt May 4, 2015

Member

I thought we were not supposed to add functions to API objects

This comment has been minimized.

@smarterclayton

smarterclayton May 4, 2015

Contributor

To the storage interface, a la the various things we pass in. A method "AcceptName(obj runtime.Object, name string) error" or something.

----- Original Message -----

    ctx := scope.ContextFunc(req)
    ctx = api.WithNamespace(ctx, namespace)
  •   if len(name) > 0 {
    
  •       ctx = api.WithValue(ctx, "name", name)
    

I thought we were not supposed to add functions to API objects


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/7718/files#r29612592

@smarterclayton

smarterclayton May 4, 2015

Contributor

To the storage interface, a la the various things we pass in. A method "AcceptName(obj runtime.Object, name string) error" or something.

----- Original Message -----

    ctx := scope.ContextFunc(req)
    ctx = api.WithNamespace(ctx, namespace)
  •   if len(name) > 0 {
    
  •       ctx = api.WithValue(ctx, "name", name)
    

I thought we were not supposed to add functions to API objects


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/7718/files#r29612592

This comment has been minimized.

@csrwng

csrwng May 4, 2015

Contributor

@smarterclayton - If that's the case :-) Could we just add a new interface that also takes the name? SubResourceCreater ? And then we just define a Create(ctx, name, obj) ?

@csrwng

csrwng May 4, 2015

Contributor

@smarterclayton - If that's the case :-) Could we just add a new interface that also takes the name? SubResourceCreater ? And then we just define a Create(ctx, name, obj) ?

This comment has been minimized.

@smarterclayton

smarterclayton May 4, 2015

Contributor

It's the Storage objects' responsibility to make that determination, by passing it the name and the object. Then the storage object can do whatever it wants, including returning an error or silently eating it.

----- Original Message -----

    ctx := scope.ContextFunc(req)
    ctx = api.WithNamespace(ctx, namespace)
  •   if len(name) > 0 {
    
  •       ctx = api.WithValue(ctx, "name", name)
    

If we set the name on the object we may be hiding an error condition. You may
be posting to a particular path and post an object with a different name. I
would think you'd want the server to tell you that you have a mismatch
instead of just taking the name from the path.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/7718/files#r29630615

@smarterclayton

smarterclayton May 4, 2015

Contributor

It's the Storage objects' responsibility to make that determination, by passing it the name and the object. Then the storage object can do whatever it wants, including returning an error or silently eating it.

----- Original Message -----

    ctx := scope.ContextFunc(req)
    ctx = api.WithNamespace(ctx, namespace)
  •   if len(name) > 0 {
    
  •       ctx = api.WithValue(ctx, "name", name)
    

If we set the name on the object we may be hiding an error condition. You may
be posting to a particular path and post an object with a different name. I
would think you'd want the server to tell you that you have a mismatch
instead of just taking the name from the path.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/7718/files#r29630615

This comment has been minimized.

@csrwng

csrwng May 4, 2015

Contributor

I think we're saying the same thing. I was thinking the new create method would take a runtime.Object and a name.

@csrwng

csrwng May 4, 2015

Contributor

I think we're saying the same thing. I was thinking the new create method would take a runtime.Object and a name.

This comment has been minimized.

@smarterclayton

smarterclayton May 4, 2015

Contributor

What about DELETE of the collection? And LIST? And WATCH? That's 3 new special methods.

----- Original Message -----

    ctx := scope.ContextFunc(req)
    ctx = api.WithNamespace(ctx, namespace)
  •   if len(name) > 0 {
    
  •       ctx = api.WithValue(ctx, "name", name)
    

I think we're saying the same thing. I was thinking the new create method
would take a runtime.Object and a name.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/7718/files#r29632731

@smarterclayton

smarterclayton May 4, 2015

Contributor

What about DELETE of the collection? And LIST? And WATCH? That's 3 new special methods.

----- Original Message -----

    ctx := scope.ContextFunc(req)
    ctx = api.WithNamespace(ctx, namespace)
  •   if len(name) > 0 {
    
  •       ctx = api.WithValue(ctx, "name", name)
    

I think we're saying the same thing. I was thinking the new create method
would take a runtime.Object and a name.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/7718/files#r29632731

This comment has been minimized.

@csrwng

csrwng May 4, 2015

Contributor

What would be the difference between get and list? Delete already gets a name passed to it. Not sure what those new methods would look like.

@csrwng

csrwng May 4, 2015

Contributor

What would be the difference between get and list? Delete already gets a name passed to it. Not sure what those new methods would look like.

This comment has been minimized.

@smarterclayton

smarterclayton May 5, 2015

Contributor

DeleteCollection

----- Original Message -----

    ctx := scope.ContextFunc(req)
    ctx = api.WithNamespace(ctx, namespace)
  •   if len(name) > 0 {
    
  •       ctx = api.WithValue(ctx, "name", name)
    

What would be the difference between get and list? Delete already gets a name
passed to it. Not sure what those new methods would look like.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/7718/files#r29633690

@smarterclayton

smarterclayton May 5, 2015

Contributor

DeleteCollection

----- Original Message -----

    ctx := scope.ContextFunc(req)
    ctx = api.WithNamespace(ctx, namespace)
  •   if len(name) > 0 {
    
  •       ctx = api.WithValue(ctx, "name", name)
    

What would be the difference between get and list? Delete already gets a name
passed to it. Not sure what those new methods would look like.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/7718/files#r29633690

@csrwng

This comment has been minimized.

Show comment
Hide comment
@csrwng

csrwng May 6, 2015

Contributor

v2 of pull ready for another review

Contributor

csrwng commented May 6, 2015

v2 of pull ready for another review

Show outdated Hide outdated pkg/api/rest/rest.go
// on collections and those that operate on individually named items.
// Collection interfaces:
// (Method: Current -> Proposed)
// GET: Lister -> CollectionLister

This comment has been minimized.

@smarterclayton

smarterclayton May 6, 2015

Contributor

CollectionGetter

List vs Get is arbitrary - a sub resource collection list is a collection get, so let's not confuse it.

@smarterclayton

smarterclayton May 6, 2015

Contributor

CollectionGetter

List vs Get is arbitrary - a sub resource collection list is a collection get, so let's not confuse it.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 6, 2015

Contributor

resthandler.go keeps getting uglier. Just the one comment about the comment.

Contributor

smarterclayton commented May 6, 2015

resthandler.go keeps getting uglier. Just the one comment about the comment.

API Server - pass path name in context of create request for subresource
Allows a REST storage for a subresource to obtain name in path from
request.
@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 6, 2015

Contributor

LGTM, will merge on green

Contributor

smarterclayton commented May 6, 2015

LGTM, will merge on green

@smarterclayton smarterclayton added the lgtm label May 6, 2015

smarterclayton added a commit that referenced this pull request May 6, 2015

Merge pull request #7718 from csrwng/create_sub_name
API Server - pass path name in context of create request for subresource

@smarterclayton smarterclayton merged commit b60a90c into kubernetes:master May 6, 2015

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Shippable Shippable builds completed
Details
cla/google All necessary CLAs are signed

@csrwng csrwng deleted the csrwng:create_sub_name branch Jul 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment