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.

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

Resolve api groups in resolveresource #8280

Merged
merged 1 commit into from
Mar 31, 2016
Merged

Resolve api groups in resolveresource #8280

merged 1 commit into from
Mar 31, 2016

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Mar 29, 2016

Fixes #7117


partialResource := unversioned.GroupVersionResource{Resource: strings.ToLower(resourceGroupParts[0])}
if len(resourceGroupParts) > 1 {
partialResource.Group = resourceGroupParts[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

lower the group while you're here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oc get hpa.Autoscaling doesn't lowercase group to work... I'm not sure I want to enable incorrect groups to work here

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed with @liggitt - at any case we should not diverge from kubectl

@liggitt
Copy link
Contributor Author

liggitt commented Mar 29, 2016

@deads2k @Kargakis PTAL

@liggitt
Copy link
Contributor Author

liggitt commented Mar 29, 2016

[test]

if groupResource != gvr.GroupResource() {
return "", "", &meta.AmbiguousResourceError{PartialResource: partialResource, MatchingResources: gvrs}
}
// Allow specifying the group the same way kubectl does
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be ideal if we would re-use the code from there for all the parsing we do here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be ideal if we would re-use the code from there for all the parsing we do here.

ParseResourceArg

@0xmichalis
Copy link
Contributor

LGTM

@liggitt
Copy link
Contributor Author

liggitt commented Mar 30, 2016

[merge]

@liggitt
Copy link
Contributor Author

liggitt commented Mar 30, 2016

flake on #6040, re[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 49ad7cc

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2605/)

@liggitt
Copy link
Contributor Author

liggitt commented Mar 30, 2016

Yum timeout, re[merge]

@0xmichalis
Copy link
Contributor

flake #6533

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5489/) (Image: devenv-rhel7_3877)

@0xmichalis
Copy link
Contributor

now network flake [merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 49ad7cc

@openshift-bot openshift-bot merged commit 7af1063 into openshift:master Mar 31, 2016
@liggitt liggitt deleted the resolve-group-resources branch April 1, 2016 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants