support editing before creating resource #33250

mengqiy · 2016-09-22T05:03:14Z

Support kubectl create -f config.yaml --edit
Support editing before creating resource from files, urls and stdin.
The behavior is similar to kubectl edit
It won't create anything when edit make no change.

partial: #18064

Based on: #33686 and #33973

Support editing before creating resource from files, urls and stdin, e.g. `kubectl create -f config.yaml --edit`
It won't create anything when edit make no change.

This change is

mengqiy · 2016-09-23T18:37:53Z

Improve edit experience a bit according #26050(comment)

a) always go back to the editor
b) always retain what I hand-edited, even if that has to be in comments

janetkuo · 2016-09-28T00:39:58Z

I tried the example command, kubectl create -f docker-registry.yaml --edit --output-version=v1 -o json. While yaml editor formatted perfectly, with -o yaml I got this one-line, unformatted output:

{"apiVersion":"v1","kind":"Pod","metadata":{"name":"test-kubectl","namespace":"default"},"spec":{"containers":[{"command":["sh","-c","sleep 5; wget -O - ${KUBERNETES_RO_SERVICE_HOST}:       ${KUBERNETES_RO_SERVICE_PORT}/api/v1/pods/; sleep 10000"],"env":[{"name":"KUBERNETES_RO_SERVICE_HOST","value":"127.0.0.1"},{"name":"KUBERNETES_RO_SERVICE_PORT","value":"8001"}],"image":     "gcr.io/google_containers/busybox","name":"busybox","ports":[{"containerPort":8080}],"volumeMounts":[{"mountPath":"/usr/local","name":"test-kubectl"}]}],"volumes":[{"hostPath":{"path":      "/usr/local/google/home/chiachenk/go/src/k8s.io/kubernetes/_output/local/bin/linux/amd64"},"name":"test-kubectl"}]}}

janetkuo · 2016-09-28T00:52:37Z

We should refactor the code in edit and try to reuse them, instead of duplicating them

mengqiy · 2016-09-28T20:17:50Z

While yaml editor formatted perfectly, with -o yaml I got this one-line, unformatted output

I create another PR #33686 to format the JSONPrintor. That PR could fix the issue you mentioned.

janetkuo · 2016-09-28T23:19:06Z

cc @kubernetes/kubectl

soltysh · 2016-09-30T09:51:13Z

We should refactor the code in edit and try to reuse them, instead of duplicating them

👍 for that idea, to make it more clear.

k8s-ci-robot · 2016-09-30T17:02:11Z

Jenkins Kubemark GCE e2e failed for commit b763b716895f7fe1a0d3644ee732de937c66281f. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-09-30T17:02:11Z

Jenkins GCE e2e failed for commit b763b716895f7fe1a0d3644ee732de937c66281f. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-09-30T17:02:41Z

Jenkins GCI GCE e2e failed for commit b763b716895f7fe1a0d3644ee732de937c66281f. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-09-30T17:02:52Z

Jenkins unit/integration failed for commit b763b716895f7fe1a0d3644ee732de937c66281f. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

mengqiy · 2016-09-30T17:11:13Z

Refactored and rebased. PTAL
If you want to try it on your local machine, make sure you has rebased, since this PR is based on #33686 that has been merged recently.

soltysh · 2016-09-30T19:39:14Z

pkg/kubectl/cmd/edit.go

+	return EditHelper(f, out, errOut, cmd, args, options, true)
+}
+
+func EditHelper(f *cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args []string, options *resource.FilenameOptions, isUsedByEditCmd bool) error {


I'd prefer runEdit

soltysh · 2016-09-30T19:42:07Z

pkg/kubectl/cmd/edit.go

+			ResourceTypeOrNameArgs(true, args...).
+			Latest()
+	} else {
+		b = resource.NewBuilder(mapper, typer, resource.ClientMapperFunc(f.UnstructuredClientForMapping), runtime.UnstructuredJSONScheme)


Both create and edit are meant to work with k8s resources, not sure why using UnstructuredClientForMapping here?

This is because I want to support the use case:
kubectl create -f invalid-k8s-resource.yaml --edit
where 'invalid-k8s-resource.yaml' contains some invalid configuration. User can edit these invalid fields before creating them. I think it will make --edit more useful.
Otherwise, it will simply return an error, the editor won't even pop out to let user edit it.

soltysh · 2016-09-30T19:47:18Z

pkg/kubectl/cmd/edit.go

-				os.Remove(file)
-				fmt.Fprintln(errOut, "Edit cancelled, no changes made.")
-				return nil
+			if isUsedByEditCmd {


I wonder all of those if isUsedByEditCmd, haven't tried but I'd like to have only the necessary required ones. I'll try to check it out on Mon.

janetkuo · 2016-10-03T18:03:41Z

pkg/kubectl/cmd/edit.go

@@ -430,9 +468,9 @@ func RunEdit(f *cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args
 				}
 				return nil
 			}
-
-			// loop again and edit the remaining items
-			infos = results.edit


Why remove this?

infos will be overwritten immediately at the start of next iteration in the for loop. See: this line
So this line is actually useless.

janetkuo · 2016-10-03T18:03:50Z

pkg/kubectl/cmd/edit.go

 			if err != nil {
-				return preservedFile(err, results.file, errOut)


why change this?

janetkuo · 2016-10-03T18:24:16Z

There are a number of changes in this PR that I'm not sure if it'll change edit behavior and cause regression. Are those changes necessary for create --edit, or are they just for improving edit? If they're not required for making create --edit work, separate them into their own commit / PR.

Also, the edit function is getting very long (300+ lines) and becomes harder to read / maintain. Is it possible to separate it into multiple smaller functions? (Let me know if it takes too much time.) Something like:

Get printer (json, yaml, etc) (should be the same for all commands)
Get their own mapper / resource mapper
Get their own resource builder / result
Edit the resource in Visit (should be the same or similar in all commands)
PrintSuccess for each command (edit, create, etc)

Also keep in mind that this edit function may be extended to other commands (such as run --edit) in the future.

adohe-zz · 2016-10-06T00:01:42Z

Also, the edit function is getting very long (300+ lines) and becomes harder to read / maintain. Is it possible to separate it into multiple smaller functions?

I think so, maybe we can open a new PR to do this.

mengqiy · 2016-10-06T01:10:25Z

I think so, maybe we can open a new PR to do this.

I will create another PR for refactoring, and it will base on #33973

janetkuo · 2016-10-14T21:54:38Z

Would you add some tests in test-cmd.sh too?

mengqiy · 2016-10-14T22:00:26Z

@janetkuo I created another PR #34862 to fix the panic issue. Sorry about introducing this issue.

@janetkuo

Automatic merge from submit-queue fix error handling Add missing error handling mentioned in [#33250 Comment](#33250 (comment)) @janetkuo

k8s-ci-robot · 2016-10-19T23:42:24Z

Jenkins GKE smoke e2e failed for commit 36c77a6e144267fda530f8502238555070e2e505. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-19T23:52:55Z

Jenkins verification failed for commit 36c77a6e144267fda530f8502238555070e2e505. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

mengqiy · 2016-10-20T02:58:24Z

Updated. PTAL
The behavior has been changed. It won't create anything when edit make no change.

mengqiy · 2016-10-22T00:34:31Z

@janetkuo ping

janetkuo · 2016-10-22T00:49:56Z

pkg/kubectl/cmd/edit.go

+const (
+	NormalEditMode       = "normal_mode"
+	EditBeforeCreateMode = "edit_before_create_mode"
+)


suggest something like

type EditMode string const ( NormalEditMode EditMode = "normal_mode" EditBeforeCreateMode EditMode = "edit_before_create_mode" )

janetkuo · 2016-10-22T00:52:54Z

hack/make-rules/test-cmd.sh

@@ -720,6 +720,24 @@ runTests() {
  # Pre-condition: no POD exists
  create_and_use_new_namespace
  kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" ''
+  ## kubectl edit can update the image field of a POD. tmp-editor.sh is a fake editor


nit: comment needs to be changed to kubectl create --edit

janetkuo · 2016-10-22T00:56:36Z

pkg/kubectl/cmd/create.go

+func RunEditOnCreate(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, options *resource.FilenameOptions) error {
+	outputFlag := cmdutil.GetFlagString(cmd, "output")
+	if outputFlag != "json" && outputFlag != "yaml" {
+		return cmdutil.UsageError(cmd, "--output must be one of json|yaml, when --edit is true")


If --edit is specified, we should set --output to either json or yaml, so that one can do kubectl create --edit without specifying -o. WDYT?

If we change this, need a test for it

Maybe choose yaml, the same as kubectl edit

Yea, if --edit is specified, we can use yaml as default when -o is not specified.

janetkuo · 2016-10-22T00:58:10Z

Overall looks good. Had a few comments

soltysh

One final comment and please squash your changes at this point already.

soltysh · 2016-10-24T15:08:41Z

hack/make-rules/test-cmd.sh

@@ -720,6 +720,24 @@ runTests() {
  # Pre-condition: no POD exists
  create_and_use_new_namespace
  kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" ''
+  ## kubectl create --edit can update the image field of a POD. tmp-editor.sh is a fake editor
+  echo -e "#!/bin/bash\n$SED -i \"s/gcr.io\/google_containers\/serve_hostname/nginx/g\" \$1" > /tmp/tmp-editor.sh


Use mktemp instead of creating a file that might conflict with something else.

mengqiy · 2016-10-24T17:00:50Z

Squashed.

soltysh

LGTM

k8s-ci-robot · 2016-10-28T17:48:06Z

Jenkins GCI GKE smoke e2e failed for commit c641834. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

mengqiy · 2016-10-28T18:23:05Z

@k8s-bot gci gke e2e test this

k8s-github-robot · 2016-10-29T18:00:55Z

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

k8s-github-robot · 2016-10-29T18:45:42Z

Automatic merge from submit-queue

googlebot added the cla: yes label Sep 22, 2016

k8s-github-robot assigned pwittrock Sep 22, 2016

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

pwittrock assigned janetkuo and unassigned pwittrock Sep 23, 2016

janetkuo added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Sep 23, 2016

mengqiy force-pushed the edit_before_create branch from 5a76c2d to b763b71 Compare September 30, 2016 16:57

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

mengqiy force-pushed the edit_before_create branch from b763b71 to 28f44cd Compare September 30, 2016 17:07

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

soltysh requested changes Sep 30, 2016

View reviewed changes

janetkuo reviewed Oct 3, 2016

View reviewed changes

soltysh mentioned this pull request Oct 6, 2016

Add a kubectl command to generate manifests #34148

Closed

mengqiy mentioned this pull request Oct 6, 2016

Refactor kubectl edit cmd #34279

Merged

mengqiy mentioned this pull request Oct 14, 2016

fix error handling #34862

Merged

k8s-github-robot pushed a commit that referenced this pull request Oct 15, 2016

Merge pull request #34862 from ymqytw/fix_edit_panic

c0fea2a

Automatic merge from submit-queue fix error handling Add missing error handling mentioned in [#33250 Comment](#33250 (comment)) @janetkuo

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

mengqiy force-pushed the edit_before_create branch from 56006dc to 36c77a6 Compare October 19, 2016 23:28

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

mengqiy force-pushed the edit_before_create branch from 36c77a6 to 32d21b7 Compare October 19, 2016 23:57

mengqiy force-pushed the edit_before_create branch from 32d21b7 to d7edbb3 Compare October 21, 2016 22:18

janetkuo reviewed Oct 22, 2016

View reviewed changes

soltysh reviewed Oct 24, 2016

View reviewed changes

support editing before creating resource

c641834

mengqiy force-pushed the edit_before_create branch from 51397fb to c641834 Compare October 24, 2016 16:57

soltysh approved these changes Oct 24, 2016

View reviewed changes

janetkuo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2016

k8s-github-robot merged commit 739f78f into kubernetes:master Oct 29, 2016

mengqiy deleted the edit_before_create branch October 29, 2016 21:01

mengqiy mentioned this pull request Nov 11, 2016

fix create --edit #36617

Closed

mengqiy mentioned this pull request Dec 2, 2016

Changes to Kubectl behavior Release Notes #34746

Closed

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

		if err != nil {
		return preservedFile(err, results.file, errOut)

support editing before creating resource #33250

support editing before creating resource #33250

Conversation

mengqiy commented Sep 22, 2016 • edited Loading

mengqiy commented Sep 23, 2016

janetkuo commented Sep 28, 2016

janetkuo commented Sep 28, 2016

mengqiy commented Sep 28, 2016

janetkuo commented Sep 28, 2016

soltysh commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

mengqiy commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo commented Oct 3, 2016 • edited Loading

adohe-zz commented Oct 6, 2016

mengqiy commented Oct 6, 2016

janetkuo commented Oct 14, 2016

mengqiy commented Oct 14, 2016 • edited Loading

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

mengqiy commented Oct 20, 2016 • edited Loading

mengqiy commented Oct 22, 2016

janetkuo Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo commented Oct 22, 2016

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented Oct 24, 2016

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 28, 2016

mengqiy commented Oct 28, 2016

k8s-github-robot commented Oct 29, 2016

k8s-github-robot commented Oct 29, 2016

mengqiy commented Sep 22, 2016 •

edited

Loading

janetkuo commented Oct 3, 2016 •

edited

Loading

mengqiy commented Oct 14, 2016 •

edited

Loading

mengqiy commented Oct 20, 2016 •

edited

Loading

janetkuo Oct 22, 2016 •

edited

Loading