-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
Support kubectl group/resource name #14461
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,16 +169,25 @@ func (m *DefaultRESTMapper) ResourceSingularizer(resource string) (singular stri | |
|
||
// VersionAndKindForResource implements RESTMapper | ||
func (m *DefaultRESTMapper) VersionAndKindForResource(resource string) (defaultVersion, kind string, err error) { | ||
if parts := strings.Split(resource, "/"); len(parts) > 1 { | ||
if len(parts) > 2 { | ||
return "", "", fmt.Errorf("resource %q has an invalid name", resource) | ||
} | ||
resource = parts[1] | ||
if m.group != parts[0] { | ||
return "", "", fmt.Errorf("no resource %q is defined for group %q", resource, m.group) | ||
} | ||
} | ||
meta, ok := m.mapping[strings.ToLower(resource)] | ||
if !ok { | ||
return "", "", fmt.Errorf("in version and kind for resource, no resource %q has been defined", resource) | ||
return "", "", fmt.Errorf("no resource %q has been defined", resource) | ||
} | ||
return meta.APIVersion, meta.Kind, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To force people adding "experimental/" before an experimental resource, maybe it's enough to add a check here, because this is the only place you check if resource contains "/". |
||
} | ||
|
||
func (m *DefaultRESTMapper) GroupForResource(resource string) (string, error) { | ||
if _, ok := m.mapping[strings.ToLower(resource)]; !ok { | ||
return "", fmt.Errorf("in group for resource, no resource %q has been defined", resource) | ||
return "", fmt.Errorf("no resource %q has been defined", resource) | ||
} | ||
return m.group, nil | ||
} | ||
|
@@ -273,6 +282,12 @@ func (m *DefaultRESTMapper) AliasesForResource(alias string) ([]string, bool) { | |
return nil, false | ||
} | ||
|
||
// ResourceIsValid takes a string (kind) and checks if it's a valid resource | ||
func (m *DefaultRESTMapper) ResourceIsValid(resource string) bool { | ||
_, _, err := m.VersionAndKindForResource(resource) | ||
return err == nil | ||
} | ||
|
||
// MultiRESTMapper is a wrapper for multiple RESTMappers. | ||
type MultiRESTMapper []RESTMapper | ||
|
||
|
@@ -335,3 +350,13 @@ func (m MultiRESTMapper) AliasesForResource(alias string) (aliases []string, ok | |
} | ||
return nil, false | ||
} | ||
|
||
// ResourceIsValid takes a string (either group/kind or kind) and checks if it's a valid resource | ||
func (m MultiRESTMapper) ResourceIsValid(resource string) bool { | ||
for _, t := range m { | ||
if t.ResourceIsValid(resource) { | ||
return true | ||
} | ||
} | ||
return false | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,12 +186,12 @@ func (b *Builder) ResourceTypes(types ...string) *Builder { | |
return b | ||
} | ||
|
||
// ResourceNames accepts a default type and one or more names, and creates tuples of | ||
// ResourceNames accepts a default type or group/type and one or more names, and creates tuples of | ||
// resources | ||
func (b *Builder) ResourceNames(resource string, names ...string) *Builder { | ||
for _, name := range names { | ||
// See if this input string is of type/name format | ||
tuple, ok, err := splitResourceTypeName(name) | ||
// See if this input string is of type/name or group/type/name format | ||
tuple, ok, err := splitGroupResourceTypeName(name) | ||
if err != nil { | ||
b.errs = append(b.errs, err) | ||
return b | ||
|
@@ -274,12 +274,47 @@ func (b *Builder) SelectAllParam(selectAll bool) *Builder { | |
return b | ||
} | ||
|
||
// ResourceTypeOrNameArgs indicates that the builder should accept arguments | ||
// of the form `(<type1>[,<type2>,...]|<type> <name1>[,<name2>,...])`. When one argument is | ||
// received, the types provided will be retrieved from the server (and be comma delimited). | ||
// When two or more arguments are received, they must be a single type and resource name(s). | ||
func (b *Builder) hasNamesArg(args []string) bool { | ||
if len(args) > 1 { | ||
return true | ||
} | ||
if len(args) != 1 { | ||
return false | ||
} | ||
// the first (and the only) arg could be (type[,group/type]), type/name, or group/type/name | ||
s := args[0] | ||
// type or group/type (no name) | ||
if strings.Contains(s, ",") { | ||
return false | ||
} | ||
// type (no name) | ||
if !strings.Contains(s, "/") { | ||
return false | ||
} | ||
// group/type/name, type/name or group/type | ||
tuple := strings.Split(s, "/") | ||
if len(tuple) != 3 && !b.mapper.ResourceIsValid(tuple[0]) { | ||
// TODO: prints warning/error message here when tuple[0] may be resource or group (name duplication)? | ||
return false | ||
} | ||
return true | ||
} | ||
|
||
func getResource(groupResource string) string { | ||
if strings.Contains(groupResource, "/") { | ||
return strings.Split(groupResource, "/")[1] | ||
} | ||
return groupResource | ||
} | ||
|
||
// ResourceTypeOrNameArgs indicates that the builder should accept arguments of the form | ||
// `(<type1>[,<type2>,...] [<name1>[ <name2> ...]]|<type1>/<name1> [<type2>/<name2>...]|)` | ||
// When one argument is received, the types provided will be retrieved from the server (and be comma delimited). | ||
// The allowEmptySelector permits to select all the resources (via Everything func). | ||
// <type> = (<group>/<resource type> | <resource type>) | ||
func (b *Builder) ResourceTypeOrNameArgs(allowEmptySelector bool, args ...string) *Builder { | ||
hasNames := b.hasNamesArg(args) | ||
|
||
// convert multiple resources to resource tuples, a,b,c d as a transform to a/d b/d c/d | ||
if len(args) >= 2 { | ||
resources := []string{} | ||
|
@@ -297,13 +332,13 @@ func (b *Builder) ResourceTypeOrNameArgs(allowEmptySelector bool, args ...string | |
} | ||
} | ||
|
||
if ok, err := hasCombinedTypeArgs(args); ok { | ||
if ok, err := hasCombinedTypeArgs(args); ok && hasNames { | ||
if err != nil { | ||
b.errs = append(b.errs, err) | ||
return b | ||
} | ||
for _, s := range args { | ||
tuple, ok, err := splitResourceTypeName(s) | ||
tuple, ok, err := splitGroupResourceTypeName(s) | ||
if err != nil { | ||
b.errs = append(b.errs, err) | ||
return b | ||
|
@@ -342,7 +377,7 @@ func (b *Builder) ResourceTypeOrNameArgs(allowEmptySelector bool, args ...string | |
func (b *Builder) replaceAliases(input string) string { | ||
replaced := []string{} | ||
for _, arg := range strings.Split(input, ",") { | ||
if aliases, ok := b.mapper.AliasesForResource(arg); ok { | ||
if aliases, ok := b.mapper.AliasesForResource(getResource(arg)); ok { | ||
arg = strings.Join(aliases, ",") | ||
} | ||
replaced = append(replaced, arg) | ||
|
@@ -360,13 +395,33 @@ func hasCombinedTypeArgs(args []string) (bool, error) { | |
switch { | ||
case hasSlash > 0 && hasSlash == len(args): | ||
return true, nil | ||
case hasSlash > 0 && hasSlash != len(args): | ||
return true, fmt.Errorf("when passing arguments in resource/name form, all arguments must include the resource") | ||
default: | ||
return false, nil | ||
} | ||
} | ||
|
||
// splitGroupResourceTypeName handles group/type/name resource formats and returns a resource tuple | ||
// (empty or not), whether it successfully found one, and an error | ||
func splitGroupResourceTypeName(s string) (resourceTuple, bool, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a unit test for this function, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
if !strings.Contains(s, "/") { | ||
return resourceTuple{}, false, nil | ||
} | ||
seg := strings.Split(s, "/") | ||
if len(seg) != 2 && len(seg) != 3 { | ||
return resourceTuple{}, false, fmt.Errorf("arguments in group/resource/name form may not have more than two slashes") | ||
} | ||
resource, name := "", "" | ||
if len(seg) == 2 { | ||
resource, name = seg[0], seg[1] | ||
} else { | ||
resource, name = fmt.Sprintf("%s/%s", seg[0], seg[1]), seg[2] | ||
} | ||
if len(resource) == 0 || len(name) == 0 || len(SplitResourceArgument(resource)) != 1 { | ||
return resourceTuple{}, false, fmt.Errorf("arguments in group/resource/name form must have a single resource and name") | ||
} | ||
return resourceTuple{Resource: resource, Name: name}, true, nil | ||
} | ||
|
||
// splitResourceTypeName handles type/name resource formats and returns a resource tuple | ||
// (empty or not), whether it successfully found one, and an error | ||
func splitResourceTypeName(s string) (resourceTuple, bool, error) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -565,6 +565,112 @@ func TestSingleResourceType(t *testing.T) { | |
} | ||
} | ||
|
||
func TestHasNamesArg(t *testing.T) { | ||
testCases := map[string]struct { | ||
args []string | ||
expected bool | ||
}{ | ||
"resource/name": { | ||
args: []string{"pods/foo"}, | ||
expected: true, | ||
}, | ||
"resource name": { | ||
args: []string{"pods", "foo"}, | ||
expected: true, | ||
}, | ||
"resource1,resource2 name": { | ||
args: []string{"pods,rc", "foo"}, | ||
expected: true, | ||
}, | ||
"resource1,group2/resource2 name": { | ||
args: []string{"pods,experimental/deployments", "foo"}, | ||
expected: true, | ||
}, | ||
"group/resource name": { | ||
args: []string{"experimental/deployments", "foo"}, | ||
expected: true, | ||
}, | ||
"group/resource/name": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about "group/resource/name,group2/resource2"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
args: []string{"experimental/deployments/foo"}, | ||
expected: true, | ||
}, | ||
"group1/resource1,group2/resource2": { | ||
args: []string{"experimental/daemonsets,experimental/deployments"}, | ||
expected: false, | ||
}, | ||
"resource1,group2/resource2": { | ||
args: []string{"pods,experimental/deployments"}, | ||
expected: false, | ||
}, | ||
"group/resource/name,group2/resource2": { | ||
args: []string{"experimental/deployments/foo,controller/deamonset"}, | ||
expected: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. False? but the first arg has a name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Users can only use comma to separate types (but not names) so it returns false in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK |
||
}, | ||
} | ||
for k, testCase := range testCases { | ||
b := NewBuilder(testapi.Default.RESTMapper(), api.Scheme, fakeClient()) | ||
if testCase.expected != b.hasNamesArg(testCase.args) { | ||
t.Errorf("%s: unexpected argument - expected: %v", k, testCase.expected) | ||
} | ||
} | ||
} | ||
|
||
func TestSplitGroupResourceTypeName(t *testing.T) { | ||
expectNoErr := func(err error) bool { return err == nil } | ||
expectErr := func(err error) bool { return err != nil } | ||
testCases := map[string]struct { | ||
arg string | ||
expectedTuple resourceTuple | ||
expectedOK bool | ||
errFn func(error) bool | ||
}{ | ||
"group/type/name": { | ||
arg: "experimental/deployments/foo", | ||
expectedTuple: resourceTuple{Resource: "experimental/deployments", Name: "foo"}, | ||
expectedOK: true, | ||
errFn: expectNoErr, | ||
}, | ||
"type/name": { | ||
arg: "pods/foo", | ||
expectedTuple: resourceTuple{Resource: "pods", Name: "foo"}, | ||
expectedOK: true, | ||
errFn: expectNoErr, | ||
}, | ||
"type": { | ||
arg: "pods", | ||
expectedOK: false, | ||
errFn: expectNoErr, | ||
}, | ||
"": { | ||
arg: "", | ||
expectedOK: false, | ||
errFn: expectNoErr, | ||
}, | ||
"/": { | ||
arg: "/", | ||
expectedOK: false, | ||
errFn: expectErr, | ||
}, | ||
"group/type/name/something": { | ||
arg: "experimental/deployments/foo/something", | ||
expectedOK: false, | ||
errFn: expectErr, | ||
}, | ||
} | ||
for k, testCase := range testCases { | ||
tuple, ok, err := splitGroupResourceTypeName(testCase.arg) | ||
if !testCase.errFn(err) { | ||
t.Errorf("%s: unexpected error: %v", k, err) | ||
} | ||
if ok != testCase.expectedOK { | ||
t.Errorf("%s: unexpected ok: %v", k, ok) | ||
} | ||
if testCase.expectedOK && !reflect.DeepEqual(tuple, testCase.expectedTuple) { | ||
t.Errorf("%s: unexpected tuple - expected: %v, got: %v", k, testCase.expectedTuple, tuple) | ||
} | ||
} | ||
} | ||
|
||
func TestResourceTuple(t *testing.T) { | ||
expectNoErr := func(err error) bool { return err == nil } | ||
expectErr := func(err error) bool { return err != nil } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a plausible place to catch this.
if m.group == "experimental" {
return an error about being explicit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will need to cherry-pick both PRs.