let patch use --local flag like `kubectl set image` #26722

deads2k · 2016-06-02T17:57:01Z

Adds the concept of a --local flag to kubectl patch. This flag is similar to kubectl set image -f --local because it will use the content of the file as the input to the patch operation instead of using the file content to file resource/name tuples.

This pull lets you run something like kubectl create deployment --dry-run -o yaml | kubectl set volume --local -f - -o yaml | kubectl patch --local -f - --patch {} | kubectl create -f -

As proof that it works, you can run against a local file just to mess around with it, but --local -f - is the most likely case.

$kubectl patch --local -f pkg/api/validation/testdata/v1/validPod.yaml --patch='{"spec": {"restartPolicy":"Never"}}'
apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: null
  labels:
    name: redis-master
  name: name
spec:
  containers:
  - args:
    - this
    - is
    - an
    - ok
    - command
    image: gcr.io/fake_project/fake_image:fake_tag
    name: master
    resources: {}
  restartPolicy: Never
status: {}

This is useful for setting rarely used, but immutable fields from kubectl create or kubectl convert without dropping to an interactive editor.

Some discussion here: #21648 (comment)

@smarterclayton @kubernetes/kubectl
@eparis @soltysh @stevekuznetsov we've talked about this separately

fabianofranz · 2016-06-02T18:25:32Z

So this is mutually exclusive with -f and kind/name arg, right? Needs to reflect that in the command usage.

fabianofranz · 2016-06-02T18:35:19Z

I'd like to somehow make it more clear that if you specify that flag, you are applying the patch to the local file, a different behavior in comparison to --filename which is read-only to apply the patch on the server. I'd suggest the flag name to be like --to-local-file=, --patch-in= or something similar. We also probably need to add a reference in the long desc.

smarterclayton · 2016-06-02T18:59:44Z

Why would we patch the local file? We shouldn't write our inputs ever.

On Thu, Jun 2, 2016 at 2:42 PM, Kubernetes Bot notifications@github.com
wrote:

GCE e2e build/test passed for commit 3e1f6dc
3e1f6dc
.

Build Log
http://pr-test.k8s.io/26722/kubernetes-pull-build-test-e2e-gce/42995/build-log.txt

Test Artifacts
https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/26722/kubernetes-pull-build-test-e2e-gce/42995/artifacts/

Internal Jenkins Results
http://goto.google.com/prkubekins/job/kubernetes-pull-build-test-e2e-gce//42995

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26722 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p1_azqlvu9dat-2xuUbA3zEK3xyDks5qHyQIgaJpZM4Is0Qz
.

stevekuznetsov · 2016-06-02T19:17:00Z

pkg/kubectl/cmd/patch.go

@@ -175,3 +228,24 @@ func RunPatch(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []stri
 	}
 	return nil
 }
+
+func getPatchedJS(patchType api.PatchType, originalJS, patchJS []byte, obj runtime.Object) ([]byte, error) {


You're not getting a patched JavaScript so I feel like the method could be two letters longer

deads2k · 2016-06-02T19:41:40Z

Why would we patch the local file? We shouldn't write our inputs ever.

@smarterclayton That was an example to prove that it works. The actual use-case runs more like kubectl create deployment --dry-run -o yaml | kubectl set volume --localfile - -o yaml | kubectl patch --local-file - --patch {} | kubectl create -f -

deads2k · 2016-06-02T19:44:26Z

I'd like to somehow make it more clear that if you specify that flag, you are applying the patch to the local file, a different behavior in comparison to --filename which is read-only to apply the patch on the server. I'd suggest the flag name to be like --to-local-file=, --patch-in= or something similar. We also probably need to add a reference in the long desc.

This is still read-only to the file. It's using it for input and outputing to stdout. I don't think we'd support something like sed -i, since this is more interesting in chained cases from kubectl create <subcommand> -o yaml | kubectl patch --local-file -. Files on disk were my example to prove that it worked.

fabianofranz · 2016-06-02T19:44:56Z

Oh, I got the purpose now, sorry.

I have mixed feelings then. Both the input and the output are different than the default behavior.

Input: instead of using it to identify the resources (with either tuples in -f or kind/name) you use it to provide the entire object content.

Output: instead of performing a patch you just print it, like --dry-run in a number of other commands.

Let me think a bit, but why couldn't you still use -f and do something like

$kubectl patch -f path/to/validPod.yaml --from-local-file --dry-run --patch='{"spec": {"restartPolicy":"Never"}}'

And you could combine it with --output to choose how to print it (json or yaml).

deads2k · 2016-06-02T19:45:02Z

Updated the description to make it more clear that the primary case is for chaining from other commands, not messing around with files on disk.

fabianofranz · 2016-06-02T19:47:58Z

Which is pretty much what you are saying, but use --dry-run to decide about just printing instead of performing the patch, and --output to decide about how to print it.

bgrant0607 · 2016-06-03T03:26:34Z

@deads2k Why "local-file"? That's very unintuitive to me.

bgrant0607 · 2016-06-03T03:30:57Z

@deads2k I highly value precedent and convention. They help users build a mental model of how the system works and to speculate and remember how to use numerous features -- remember the patterns, not just the specifics.

What is similar? Template files.
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/resource_printer.go#L85

I'd say a --patch-file option should replace --patch entirely.

What other commands need something like this?

bgrant0607 · 2016-06-03T03:32:22Z

cc @caesarxuchao, since he's worked on patch

deads2k · 2016-06-03T11:38:24Z

What other commands need something like this?

label, annotate, scale, set <subcommand> are the high value ones that I can think of. Allowing those commands to pipe cleanly will enable natural flows. The first time a user works with it, they'll probably do a kubectl create something, then later a kubectl set volume or some such. After a few tries, they'll then try to build a piped flow to create it in one shot: kubectl create something -o yaml | kubectl set volume --local-file - | kubectl create -f -.

Allowing pipes like this will make it easy to make something (minimal flags on create), easy to modify it (our existing mutation commands), and easy to transition to a compositional flow (--local-file). Using composition will keep each individual command relatively small.

lavalamp · 2016-06-03T19:08:53Z

We've spent a fair amount of effort moving things out of kubectl so they'll be available to all clients, so I'm only on board with this if it's a specific interface that only makes sense for users of kubectl.

I agree that --local-file is clumsy. -f means "input file" for most of our commands so, how about an --output or -o flag instead-- default "" would mean to server, but passing '-' would mean write to stdout.

lavalamp · 2016-06-03T19:11:08Z

I also agree that a templating system has significant benefits. Templates can be versioned & upgraded, and I expect them to be much more readable and maintainable than imperative chains of kubectl calls. Basically the | chain looks like a big anti pattern to me, I'd be very unhappy to inherit an application deployment that was created that way.

deads2k · 2016-06-03T19:26:29Z

Basically the | chain looks like a big anti pattern to me, I'd be very unhappy to inherit an application deployment that was created that way.

I'm not envisioning this as a way to build a script. I'm seeing it as a way to progress from "I know a few bits of this system" to "I speak runtime.Object json" without having to take a big leap.

I think the most obvious usage is probably kubect create <resource> -o yaml | kubectl set <specific thing with tailored flags> --local-file - | kubectl create -f -. -f in our normal pattern doesn't work for this, since -f means "pull resource/name tuples and get resources from the server", not "read this file and use it for input". kubectl patch just happened to be the one I wanted yesterday.

caesarxuchao · 2016-06-03T20:00:34Z

If this pipe line syntax is not intended to build a script, then I guess editing the config in an interactive editor doesn't seem to be any worse than typing such a long command.

eparis · 2016-06-03T20:06:38Z

I kinda feel like
kubectl set -f - --local
fits the systems better than
kubectl set --local-file -

eparis · 2016-06-06T13:00:19Z

I should say I do like -o or --output or --out-file or something...

smarterclayton · 2016-06-06T15:04:02Z

I don't think we should override -o because you may want JSON or YAML
output in this behavior. I agree with --local in that it's a natural step
for the use case the user will use.

On Mon, Jun 6, 2016 at 9:00 AM, Eric Paris notifications@github.com wrote:

I should say I do like -o or --output or --out-file or something...

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26722 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p8b9LpL4O5Q9LhCKPeQ1dWskSlA2ks5qJBnzgaJpZM4Is0Qz
.

smarterclayton · 2016-06-06T15:05:52Z

It may look like an anti pattern, but the alternative is sed, which I think
is a worse anti pattern. We see this coming up fairly often in real use
cases from ops teams (they want to create something, but mutate it), and
they want to use the generator pattern to handle simple cases. Forcing
them into a full templated API object is a big step, and defeats the
purpose of generators (which is to form small APIs for common use cases).

On Fri, Jun 3, 2016 at 3:11 PM, Daniel Smith notifications@github.com
wrote:

I also agree that a templating system has significant benefits. Templates
can be versioned & upgraded, and I expect them to be much more readable and
maintainable than imperative chains of kubectl calls. Basically the | chain
looks like a big anti pattern to me, I'd be very unhappy to inherit an
application deployment that was created that way.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26722 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p4Z_xvXACxjHSwtYnRvynu_lHGVJks5qIHxegaJpZM4Is0Qz
.

janetkuo · 2016-06-08T17:57:23Z

I kinda feel like
kubectl set -f - --local
fits the systems better than
kubectl set --local-file -

FWIW we already have kubectl set image -f --local in the system

deads2k · 2016-06-09T13:23:25Z

FWIW we already have kubectl set image -f --local in the system

Great, I'll update to that when I'm back on Monday.

deads2k · 2016-06-14T18:38:33Z

Updated to --local to match kubectl set image and test added. Also updated the output flags to allow encoding choices.

0xmichalis · 2016-06-15T09:28:48Z

LGTM

k8s-bot · 2016-06-21T03:41:15Z

GCE e2e build/test passed for commit ac64404.

janetkuo · 2016-06-24T21:43:05Z

this deserves a release note

k8s-bot · 2016-06-25T04:07:07Z

GCE e2e build/test passed for commit ac64404.

k8s-github-robot · 2016-06-25T16:26:01Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-06-25T16:57:21Z

GCE e2e build/test passed for commit ac64404.

k8s-github-robot · 2016-06-25T17:00:41Z

Automatic merge from submit-queue

googlebot added the cla: yes label Jun 2, 2016

k8s-github-robot assigned bgrant0607 Jun 2, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jun 2, 2016

stevekuznetsov reviewed Jun 2, 2016
View reviewed changes

bgrant0607 assigned janetkuo and unassigned bgrant0607 Jun 3, 2016

deads2k force-pushed the local-patch branch from 3e1f6dc to dd13c74 Compare June 14, 2016 18:36

deads2k changed the title ~~[POC] let patch use local file content to mutate~~ let patch use --local flag like kubectl set image Jun 14, 2016

let patch use local file content to mutate

ac64404

deads2k force-pushed the local-patch branch from dd13c74 to ac64404 Compare June 14, 2016 18:38

0xmichalis added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 15, 2016

janetkuo added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 24, 2016

k8s-github-robot merged commit 951b591 into kubernetes:master Jun 25, 2016

deads2k deleted the local-patch branch September 6, 2016 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

let patch use --local flag like `kubectl set image` #26722

let patch use --local flag like `kubectl set image` #26722

deads2k commented Jun 2, 2016 •

edited

Loading

fabianofranz commented Jun 2, 2016

fabianofranz commented Jun 2, 2016

smarterclayton commented Jun 2, 2016

stevekuznetsov Jun 2, 2016

deads2k commented Jun 2, 2016

deads2k commented Jun 2, 2016

fabianofranz commented Jun 2, 2016

deads2k commented Jun 2, 2016

fabianofranz commented Jun 2, 2016

bgrant0607 commented Jun 3, 2016

bgrant0607 commented Jun 3, 2016

bgrant0607 commented Jun 3, 2016

deads2k commented Jun 3, 2016

lavalamp commented Jun 3, 2016

lavalamp commented Jun 3, 2016

deads2k commented Jun 3, 2016

caesarxuchao commented Jun 3, 2016

eparis commented Jun 3, 2016

eparis commented Jun 6, 2016

smarterclayton commented Jun 6, 2016

smarterclayton commented Jun 6, 2016

janetkuo commented Jun 8, 2016

deads2k commented Jun 9, 2016

deads2k commented Jun 14, 2016

0xmichalis commented Jun 15, 2016

k8s-bot commented Jun 21, 2016

janetkuo commented Jun 24, 2016

k8s-bot commented Jun 25, 2016

k8s-github-robot commented Jun 25, 2016

k8s-bot commented Jun 25, 2016

k8s-github-robot commented Jun 25, 2016

let patch use --local flag like kubectl set image #26722

let patch use --local flag like kubectl set image #26722

Conversation

deads2k commented Jun 2, 2016 • edited Loading

fabianofranz commented Jun 2, 2016

fabianofranz commented Jun 2, 2016

smarterclayton commented Jun 2, 2016

stevekuznetsov Jun 2, 2016

Choose a reason for hiding this comment

deads2k commented Jun 2, 2016

deads2k commented Jun 2, 2016

fabianofranz commented Jun 2, 2016

deads2k commented Jun 2, 2016

fabianofranz commented Jun 2, 2016

bgrant0607 commented Jun 3, 2016

bgrant0607 commented Jun 3, 2016

bgrant0607 commented Jun 3, 2016

deads2k commented Jun 3, 2016

lavalamp commented Jun 3, 2016

lavalamp commented Jun 3, 2016

deads2k commented Jun 3, 2016

caesarxuchao commented Jun 3, 2016

eparis commented Jun 3, 2016

eparis commented Jun 6, 2016

smarterclayton commented Jun 6, 2016

smarterclayton commented Jun 6, 2016

janetkuo commented Jun 8, 2016

deads2k commented Jun 9, 2016

deads2k commented Jun 14, 2016

0xmichalis commented Jun 15, 2016

k8s-bot commented Jun 21, 2016

janetkuo commented Jun 24, 2016

k8s-bot commented Jun 25, 2016

k8s-github-robot commented Jun 25, 2016

k8s-bot commented Jun 25, 2016

k8s-github-robot commented Jun 25, 2016

let patch use --local flag like `kubectl set image` #26722

let patch use --local flag like `kubectl set image` #26722

deads2k commented Jun 2, 2016 •

edited

Loading