Add Local and Unstructured resource builder attributes and handle <rsrsc> / <name> pairs when --local is set #48763

juanvallejo · 2017-07-11T14:44:24Z

Release note:

NONE

Related downstream BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1390139

Adds an error message if a non-filename (or stdin) resource is specified with the --local flag in any of the kube set ... sub-commands.

cc @fabianofranz @kubernetes/sig-cli-misc

k8s-ci-robot · 2017-07-11T14:44:32Z

Hi @juanvallejo. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fabianofranz · 2017-07-11T17:59:28Z

/ok-to-test

juanvallejo · 2017-07-19T21:22:12Z

@fabianofranz wondering if this is good to go? Thanks!

fabianofranz · 2017-07-19T22:19:43Z

@juanvallejo This works for me, but here goes an idea. I didn't check the feasibility so it may not even work.

Anyways: would it make sense to make "local" an attribute of the Builder like every other (e.g. builder.Local(local).SelectorParam(...)..., then in the DisabledClientForMapping, instead of returning nil, nil in ClientForMapping you'd return a new RESTClient that encapsulates a logic that, anytime it would try to make an API call, it would return an error?

Main concept of the idea would be to rely on the actual client and, in any tentative of an API call, throw the error. Would fix it for any new commands we add in future, an avoid the need for so many conditionals.

fabianofranz · 2017-07-19T22:39:06Z

Would fix it for any new commands we add in future

Also I remember fixing a few days ago a panic related to that nil, nil and some obscure corner case. :)

juanvallejo · 2017-07-20T19:22:37Z

@fabianofranz

Anyways: would it make sense to make "local" an attribute of the Builder like every other (e.g. builder.Local(local).SelectorParam(...)..., then in the DisabledClientForMapping, instead of returning nil, nil in ClientForMapping you'd return a new RESTClient that encapsulates a logic that, anytime it would try to make an API call, it would return an error?

I think this would certainly improve the way we would handle local command invocations.
Will give it a try as part of this PR

juanvallejo · 2017-08-02T20:33:50Z

@fabianofranz ecfa394 adds a new Local and Unstructured attribute, removing the need for NewUnstructuredBuilder factory method. PTAL

juanvallejo · 2017-08-02T20:34:40Z

pkg/kubectl/resource/mapper.go

@@ -32,7 +32,7 @@ type DisabledClientForMapping struct {
 }

 func (f DisabledClientForMapping) ClientForMapping(mapping *meta.RESTMapping) (RESTClient, error) {
-	return nil, nil
+	return nil, fmt.Errorf("attempt to make api call using a disabled client")


@fabianofranz not sure if this is the right approach, or if it's better to just make and return a fake RESTClient

EDIT: Implementing a disabled RESTClient would be a bit tricky as each of its methods returns a k8s.io/client-go/rest#Request. This means that I would either have to return a request with most of its data missing, which would then return an error due to missing info when its methods are called, or I could return nil for each of the disabled RESTClient's methods. This would then cause a panic in this part of the code

juanvallejo · 2017-08-03T15:12:01Z

/test pull-kubernetes-unit
/test pull-kubernetes-verify
/test pull-kubernetes-bazel

juanvallejo · 2017-08-03T19:51:21Z

@fabianofranz Not sure how to fix bazel test. Getting the error:

missing strict dependencies:
W0803 19:38:31.906] 	pkg/kubectl/resource/builder.go: import of k8s.io/apimachinery/pkg/apis/meta/v1/unstructured, which is not a direct dependency

Tried adding this line to pkg/kubectl/resource/BUILD but that does not seem to fix it:
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library"

juanvallejo · 2017-08-31T18:36:54Z

@fabianofranz had to rebase to bring in the set_env cmd and update the resource builder there. Wondering if you can take one more look at this? Thanks!

juanvallejo · 2017-08-31T20:34:53Z

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-unit

cblecker · 2017-09-02T02:52:12Z

/unassign

Moves DisabledClientMapperForMapping wrapper to new Local attribute. Removes Factory#NewUnstructuredBuilder in favor of new Unstructured builder attribute.

juanvallejo · 2017-09-08T14:44:35Z

/retest

juanvallejo · 2017-09-08T15:44:32Z

/test pull-kubernetes-kubemark-e2e-gce

juanvallejo · 2017-09-08T18:12:07Z

@cblecker or @fabianofranz wondering if you could /lgtm this once more? Thanks!

fabianofranz · 2017-09-11T16:05:12Z

/lgtm

k8s-github-robot · 2017-09-11T16:05:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabianofranz, juanvallejo

Associated issue requirement bypassed by: fabianofranz

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubectl/OWNERS~~ [fabianofranz]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

juanvallejo · 2017-09-15T22:33:06Z

/test all

fejta-bot · 2017-09-16T05:51:00Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-09-16T09:00:04Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-09-16T13:54:02Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-09-23T04:20:52Z

Automatic merge from submit-queue (batch tested with PRs 47806, 49539, 48763, 47049, 50600). If you want to cherry-pick this change to another branch, please follow the instructions here..

smarterclayton · 2017-10-24T01:59:29Z

pkg/kubectl/resource/builder.go

+// Unstructured updates the builder's ClientMapper, RESTMapper,
+// ObjectTyper, and codec for working with unstructured api objects
+func (b *Builder) Unstructured(mapperFunc ClientMapperFunc, mapper meta.RESTMapper, typer runtime.ObjectTyper) *Builder {
+	if b.isLocal {


This is not the right place to check this. Local and unstructured have nothing to do with each other. Local commands should absolutely be able to function without needing to have compiled in objects.

@smarterclayton Thanks, will make a patch to address this

@smarterclayton

…urce-builder Automatic merge from submit-queue (batch tested with PRs 54399, 54557, 54506). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. remove mutual exclusivity check - local, unstructured builder attrs **Release note**: ```release-note NONE ``` Remove incorrect mutual exclusivity check between the resource builder `Local` and `Unstructured` attributes. Related comment: #48763 (comment) Followup to #48763 cc @smarterclayton

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 11, 2017

k8s-github-robot assigned fabianofranz Jul 11, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 11, 2017

juanvallejo force-pushed the jvallejo/update-set-image-local-error-message branch 2 times, most recently from 2580ccc to 8172607 Compare July 11, 2017 15:06

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 11, 2017

juanvallejo force-pushed the jvallejo/update-set-image-local-error-message branch from 8172607 to a07f1d7 Compare July 11, 2017 20:08

juanvallejo force-pushed the jvallejo/update-set-image-local-error-message branch from a07f1d7 to dcb86ae Compare August 2, 2017 20:32

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 2, 2017

juanvallejo commented Aug 2, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/update-set-image-local-error-message branch from dcb86ae to 0d0f723 Compare August 2, 2017 21:28

juanvallejo changed the title ~~add err message if <rsrc>/<name> pairs specified under --local~~ Add Local and Unstructured resource builder attributes and handle <rsrsc> / <name> pairs when --local is set Aug 3, 2017

juanvallejo force-pushed the jvallejo/update-set-image-local-error-message branch 5 times, most recently from 7ec02b0 to 07b444a Compare August 3, 2017 19:28

juanvallejo force-pushed the jvallejo/update-set-image-local-error-message branch from 4d2fceb to 68a3c1b Compare August 3, 2017 21:29

k8s-ci-robot assigned cblecker Aug 31, 2017

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017

juanvallejo force-pushed the jvallejo/update-set-image-local-error-message branch from 91e3f6a to a799172 Compare August 31, 2017 18:35

k8s-ci-robot unassigned cblecker Sep 2, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2017

add err message if <rsrc>/<name> pairs specified under --local

c10f4f7

juanvallejo force-pushed the jvallejo/update-set-image-local-error-message branch from a799172 to b2ff055 Compare September 5, 2017 15:46

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2017

juanvallejo added 2 commits September 5, 2017 11:57

add Local and Unstructured builder attributes

90d76ad

Moves DisabledClientMapperForMapping wrapper to new Local attribute. Removes Factory#NewUnstructuredBuilder in favor of new Unstructured builder attribute.

update bazel

04d319d

juanvallejo force-pushed the jvallejo/update-set-image-local-error-message branch from b2ff055 to 04d319d Compare September 5, 2017 15:57

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2017

k8s-github-robot merged commit 52cb897 into kubernetes:master Sep 23, 2017

juanvallejo deleted the jvallejo/update-set-image-local-error-message branch September 25, 2017 14:17

smarterclayton reviewed Oct 24, 2017

View reviewed changes

juanvallejo mentioned this pull request Oct 24, 2017

remove mutual exclusivity check - local, unstructured builder attrs #54506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Local and Unstructured resource builder attributes and handle <rsrsc> / <name> pairs when --local is set #48763

Add Local and Unstructured resource builder attributes and handle <rsrsc> / <name> pairs when --local is set #48763

juanvallejo commented Jul 11, 2017 •

edited

k8s-ci-robot commented Jul 11, 2017

fabianofranz commented Jul 11, 2017

juanvallejo commented Jul 19, 2017

fabianofranz commented Jul 19, 2017

fabianofranz commented Jul 19, 2017

juanvallejo commented Jul 20, 2017

juanvallejo commented Aug 2, 2017

juanvallejo Aug 2, 2017 •

edited

juanvallejo commented Aug 3, 2017

juanvallejo commented Aug 3, 2017

juanvallejo commented Aug 31, 2017

juanvallejo commented Aug 31, 2017

cblecker commented Sep 2, 2017

juanvallejo commented Sep 8, 2017

juanvallejo commented Sep 8, 2017

juanvallejo commented Sep 8, 2017

fabianofranz commented Sep 11, 2017

k8s-github-robot commented Sep 11, 2017

juanvallejo commented Sep 15, 2017

fejta-bot commented Sep 16, 2017

fejta-bot commented Sep 16, 2017

fejta-bot commented Sep 16, 2017

k8s-github-robot commented Sep 23, 2017

smarterclayton Oct 24, 2017

juanvallejo Oct 24, 2017

Add Local and Unstructured resource builder attributes and handle <rsrsc> / <name> pairs when --local is set #48763

Add Local and Unstructured resource builder attributes and handle <rsrsc> / <name> pairs when --local is set #48763

Conversation

juanvallejo commented Jul 11, 2017 • edited

k8s-ci-robot commented Jul 11, 2017

fabianofranz commented Jul 11, 2017

juanvallejo commented Jul 19, 2017

fabianofranz commented Jul 19, 2017

fabianofranz commented Jul 19, 2017

juanvallejo commented Jul 20, 2017

juanvallejo commented Aug 2, 2017

juanvallejo Aug 2, 2017 • edited

Choose a reason for hiding this comment

juanvallejo commented Aug 3, 2017

juanvallejo commented Aug 3, 2017

juanvallejo commented Aug 31, 2017

juanvallejo commented Aug 31, 2017

cblecker commented Sep 2, 2017

juanvallejo commented Sep 8, 2017

juanvallejo commented Sep 8, 2017

juanvallejo commented Sep 8, 2017

fabianofranz commented Sep 11, 2017

k8s-github-robot commented Sep 11, 2017

juanvallejo commented Sep 15, 2017

fejta-bot commented Sep 16, 2017

fejta-bot commented Sep 16, 2017

fejta-bot commented Sep 16, 2017

k8s-github-robot commented Sep 23, 2017

smarterclayton Oct 24, 2017

Choose a reason for hiding this comment

juanvallejo Oct 24, 2017

Choose a reason for hiding this comment

juanvallejo commented Jul 11, 2017 •

edited

juanvallejo Aug 2, 2017 •

edited