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

allow resource.version.group in kubectl #22853

Merged
merged 1 commit into from
Mar 11, 2016
Merged

allow resource.version.group in kubectl #22853

merged 1 commit into from
Mar 11, 2016
+49 −14

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 11, 2016

This allows, but does not require, kubectl get resource.version.group.com. It does a greedy match on potential versions and compares against the local RESTMapper for an exact match. If such a match exists it is chosen, if not, then it attempts a partial match assuming just resource.group. If no group exists, it falls back to old behavior.

You lose losing autonegotiation in the case where someone has "group/v1" and "group.v1/v1". It makes usage more painful, but not impossible since greedy on version will prefer "group/v1" for "resource.v1.group", but if you look up the version (or know it), you can ask for "resource.v1.v1.group"

@lavalamp @kubernetes/kubectl @liggitt @smarterclayton @kubernetes/rh-ux

@deads2k
Copy link
Contributor Author

deads2k commented Mar 11, 2016

Adding cherrypick-candidate since this is required to disable the --api-version flag that @lavalamp wants to do.

This was referenced Mar 11, 2016
Closed
Closed
@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 11, 2016
@bgrant0607
Copy link
Member

@yifan-gu Have you noticed that rkt test flaking before?

@k8s-bot
Copy link

k8s-bot commented Mar 11, 2016

GCE e2e build/test passed for commit 5bd161a.

@bgrant0607 bgrant0607 added this to the v1.2 milestone Mar 11, 2016
@ncdc
Copy link
Member

ncdc commented Mar 11, 2016

@bgrant0607 @yifan-gu the rkt_test failure is also showing up in #22851. It is trying to do an equality test on 2 arrays of Pods. I wonder if something changed in go 1.5.3? (the build appears to be using that and not 1.4.2)

@bgrant0607 bgrant0607 mentioned this pull request Mar 11, 2016
Closed
@k8s-github-robot
Copy link

@bgrant0607
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@bgrant0607
Copy link
Member

@k8s-bot unit test this issue: #22856

@bgrant0607
Copy link
Member

Wow, that was pretty hostile. Did the bot delete my comment?

@liggitt
Copy link
Member

liggitt commented Mar 11, 2016

Did the bot delete my comment?

Yeah. First github comments, then world domination.

@bgrant0607 bgrant0607 assigned lavalamp and unassigned bgrant0607 Mar 11, 2016
@bgrant0607
Copy link
Member

@lavalamp Please notify me once this is lgtm

@lavalamp
Copy link
Member

reading it now

lavalamp
lavalamp reviewed Mar 11, 2016
View reviewed changes
pkg/kubectl/resource/builder.go
@@ -433,20 +433,36 @@ func (b *Builder) SingleResourceType() *Builder {
return b
}

// mappingFor returns the RESTMapping for the Kind referenced by the resource.
// prefers a fully specified GroupVersionResource match. If we don't have one match on GroupResource
Copy link
Member

Choose a reason for hiding this comment

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

nit: cliffhanger

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2016
@lavalamp
Copy link
Member

LGTM, thanks for quick patch. (Don't fix my nit! I want to merge as-is)

@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 11, 2016
@bgrant0607
Copy link
Member

@k8s-oncall @eparis Manually merging

bgrant0607 added a commit that referenced this pull request Mar 11, 2016
@bgrant0607
Merge pull request #22853 from deads2k/parse-version
5cc7790 
allow resource.version.group in kubectl
@bgrant0607 bgrant0607 merged commit 5cc7790 into kubernetes:master Mar 11, 2016
@liggitt
Copy link
Member

liggitt commented Mar 11, 2016

would be nice to add a torture test like this to make sure args parse correctly and map to output versions correctly:

... create a single HPA
kubectl get hpa,hpa.extensions,hpa.v1beta1.extensions,hpa.autoscaling,hpa.v1.autoscaling -o ...
... verify we get 5 HPA elements output, with correct apiVersion set on each one

@lavalamp
Copy link
Member

Agree, a test is good. @deads2k can you make a follow up? If it turns up
issues we can cherry pick it as a bug fix. :)

On Fri, Mar 11, 2016 at 11:32 AM, Jordan Liggitt notifications@github.com
wrote:

would be nice to add a torture test like this to make sure args parse
correctly and map to output versions correctly:

... create a single HPA
kubectl get hpa,hpa.extensions,hpa.v1beta1.extensions,hpa.autoscaling,hpa.v1.autoscaling -o ...
... verify we get 5 HPA elements output, with correct apiVersion set on each one

—
Reply to this email directly or view it on GitHub
#22853 (comment)
.

eparis pushed a commit to eparis/kubernetes that referenced this pull request Mar 11, 2016
@bgrant0607 @eparis
Merge pull request kubernetes#22853 from deads2k/parse-version
a0566b8 
allow resource.version.group in kubectl
@eparis eparis mentioned this pull request Mar 11, 2016
Merged
@deads2k
Copy link
Contributor Author

deads2k commented Mar 11, 2016

Agree, a test is good. @deads2k can you make a follow up? If it turns up
issues we can cherry pick it as a bug fix. :)

Yeah, but I feel like I should do it with sed just for @liggitt

@eparis eparis removed cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Mar 11, 2016
@eparis
Copy link
Contributor

eparis commented Mar 11, 2016

This PR is included in #22874 which is slated to be included in the release-1.2 branch.
Please verify that the cherry-pick in that PR looks correct.

@nikhiljindal nikhiljindal mentioned this pull request Mar 11, 2016
Closed
@nikhiljindal nikhiljindal added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 17, 2016
@deads2k deads2k deleted the parse-version branch September 6, 2016 17:22
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@bgrant0607 @eparis
Merge pull request kubernetes#22853 from deads2k/parse-version
f0f35ff 
allow resource.version.group in kubectl
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@bgrant0607 @eparis
Merge pull request kubernetes#22853 from deads2k/parse-version
7d226b4 
allow resource.version.group in kubectl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.