kubectl: a couple of edit fixes

Fix some edit bugs:
* Reload the file in case of syntax errors
* Fix error format for validation errors
* Avoid hotlooping in case of no changes or empty saves

pkg/kubectl/cmd/edit.go

@@ -19,7 +19,6 @@ package cmd
 import (
 	"bufio"
 	"bytes"
-	"encoding/json"
 	"fmt"
 	"io"
 	"os"
@@ -151,24 +150,32 @@ func RunEdit(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []strin
 	}
 
 	encoder := f.JSONEncoder()
-
-	windowsLineEndings := cmdutil.GetFlagBool(cmd, "windows-line-endings")
-	edit := editor.NewDefaultEditor(f.EditorEnvs())
 	defaultVersion, err := cmdutil.OutputVersion(cmd, clientConfig.GroupVersion)
 	if err != nil {
 		return err
 	}
-	results := editResults{}
+	objs, err := resource.AsVersionedObjects(infos, defaultVersion.String(), encoder)
-	for {
+	if err != nil {
-		objs, err := resource.AsVersionedObjects(infos, defaultVersion.String(), encoder)
+		return err
-		if err != nil {
+	}
-			return preservedFile(err, results.file, out)
-		}
-		// if input object is a list, traverse and edit each item one at a time
-		for _, obj := range objs {
-			// TODO: add an annotating YAML printer that can print inline comments on each field,
-			//   including descriptions or validation errors
 
+	var (
+		windowsLineEndings = cmdutil.GetFlagBool(cmd, "windows-line-endings")
+		edit               = editor.NewDefaultEditor(f.EditorEnvs())
+		results            = editResults{}
+		original           = []byte{}
+		edited             = []byte{}
+		file               string
+	)
+
+outter:
+	for i := range objs {
+		obj := objs[i]
+		// some bookkeeping
+		results.header.flush()
+		containsError := false
+
+		for {
 			// generate the file to edit
 			buf := &bytes.Buffer{}
 			var w io.Writer = buf
@@ -178,45 +185,54 @@ func RunEdit(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []strin
 			if err := results.header.writeTo(w); err != nil {
 				return preservedFile(err, results.file, out)
 			}
-			if err := printer.PrintObj(obj, w); err != nil {
+			if !containsError {
-				return preservedFile(err, results.file, out)
+				if err := printer.PrintObj(obj, w); err != nil {
+					return preservedFile(err, results.file, out)
+				}
+				original = buf.Bytes()
+			} else {
+				// In case of an error, preserve the edited file.
+				// Remove the comments (header) from it since we already
+				// have included the latest header in the buffer above.
+				buf.Write(manualStrip(edited))
 			}
-			original := buf.Bytes()
 
 			// launch the editor
-			edited, file, err := edit.LaunchTempFile("kubectl-edit-", ext, buf)
+			editedDiff := edited
+			edited, file, err = edit.LaunchTempFile(fmt.Sprintf("%s-edit-", os.Args[0]), ext, buf)
 			if err != nil {
 				return preservedFile(err, results.file, out)
 			}
+			if bytes.Equal(stripComments(editedDiff), stripComments(edited)) {
+				// Ugly hack right here. We will hit this either (1) when we try to
+				// save the same changes we tried to save in the previous iteration
+				// which means our changes are invalid or (2) when we exit the second
+				// time. The second case is more usual so we can probably live with it.
+				// TODO: A less hacky fix would be welcome :)
+				fmt.Fprintln(out, "Edit cancelled, no valid changes were saved.")
+				continue outter
+			}
 
 			// cleanup any file from the previous pass
 			if len(results.file) > 0 {
 				os.Remove(results.file)
 			}
-
 			glog.V(4).Infof("User edited:\n%s", string(edited))
+
+			// Compare content without comments
+			if bytes.Equal(stripComments(original), stripComments(edited)) {
+				os.Remove(file)
+				fmt.Fprintln(out, "Edit cancelled, no changes made.")
+				continue outter
+			}
 			lines, err := hasLines(bytes.NewBuffer(edited))
 			if err != nil {
 				return preservedFile(err, file, out)
 			}
-			// Compare content without comments
-			if bytes.Equal(stripComments(original), stripComments(edited)) {
-				if len(results.edit) > 0 {
-					preservedFile(nil, file, out)
-				} else {
-					os.Remove(file)
-				}
-				fmt.Fprintln(out, "Edit cancelled, no changes made.")
-				continue
-			}
 			if !lines {
-				if len(results.edit) > 0 {
+				os.Remove(file)
-					preservedFile(nil, file, out)
-				} else {
-					os.Remove(file)
-				}
 				fmt.Fprintln(out, "Edit cancelled, saved file was empty.")
-				continue
+				continue outter
 			}
 
 			results = editResults{
@@ -226,8 +242,13 @@ func RunEdit(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []strin
 			// parse the edited file
 			updates, err := resourceMapper.InfoForData(edited, "edited-file")
 			if err != nil {
-				return fmt.Errorf("The edited file had a syntax error: %v", err)
+				// syntax error
+				containsError = true
+				results.header.reasons = append(results.header.reasons, editReason{head: fmt.Sprintf("The edited file had a syntax error: %v", err)})
+				continue
 			}
+			// not a syntax error as it turns out...
+			containsError = false
 
 			// put configuration annotation in "updates"
 			if err := kubectl.CreateOrUpdateAnnotation(cmdutil.GetFlagBool(cmd, cmdutil.ApplyAnnotationsFlag), updates, encoder); err != nil {
@@ -239,8 +260,8 @@ func RunEdit(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []strin
 					return err
 				}
 			}
-			// encode updates back to "edited" since we'll only generate patch from "edited"
+			editedCopy := edited
-			if edited, err = runtime.Encode(encoder, updates.Object); err != nil {
+			if editedCopy, err = runtime.Encode(encoder, updates.Object); err != nil {
 				return preservedFile(err, file, out)
 			}
 
@@ -256,7 +277,7 @@ func RunEdit(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []strin
 			if err != nil {
 				return preservedFile(err, file, out)
 			}
-			editedJS, err := yaml.ToJSON(edited)
+			editedJS, err := yaml.ToJSON(editedCopy)
 			if err != nil {
 				return preservedFile(err, file, out)
 			}
@@ -282,53 +303,37 @@ func RunEdit(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []strin
 			err = visitor.Visit(func(info *resource.Info, err error) error {
 				patched, err := resource.NewHelper(info.Client, info.Mapping).Patch(info.Namespace, info.Name, api.StrategicMergePatchType, patch)
 				if err != nil {
-					fmt.Fprintln(out, results.addError(err, info))
+					glog.V(4).Infof(results.addError(err, info))
-					return nil
+					return err
 				}
 				info.Refresh(patched, true)
 				cmdutil.PrintSuccess(mapper, false, out, info.Mapping.Resource, info.Name, "edited")
 				return nil
 			})
-			if err != nil {
+			if err == nil {
-				return preservedFile(err, file, out)
+				os.Remove(file)
+				continue outter
 			}
-
+			// Handle all possible errors
+			//
+			// 1. retryable: propose kubectl replace -f
+			// 2. notfound: indicate the location of the saved configuration of the deleted resource
+			// 3. invalid: retry those on the spot by looping ie. reloading the editor
 			if results.retryable > 0 {
-				fmt.Fprintf(out, "You can run `kubectl replace -f %s` to try this update again.\n", file)
+				fmt.Fprintf(out, "You can run `%s replace -f %s` to try this update again.\n", os.Args[0], file)
-				return errExit
+				continue outter
 			}
-			if results.conflict > 0 {
+			if results.notfound > 0 {
-				fmt.Fprintf(out, "You must update your local resource version and run `kubectl replace -f %s` to overwrite the remote changes.\n", file)
+				fmt.Fprintf(out, "The edits you made on deleted resources have been saved to %q\n", file)
-				return errExit
+				continue outter
-			}
-			if len(results.edit) == 0 {
-				if results.notfound == 0 {
-					os.Remove(file)
-				} else {
-					fmt.Fprintf(out, "The edits you made on deleted resources have been saved to %q\n", file)
-				}
 			}
+			// validation error
+			containsError = true
 		}
-		if len(results.edit) == 0 {
-			return nil
-		}
-
-		// loop again and edit the remaining items
-		infos = results.edit
 	}
 	return nil
 }
 
-// print json file (such as patch file) content for debugging
-func printJson(out io.Writer, file []byte) error {
-	diff := make(map[string]interface{})
-	if err := json.Unmarshal(file, &diff); err != nil {
-		return err
-	}
-	fmt.Fprintf(out, "%v\n", diff)
-	return nil
-}
-
 // editReason preserves a message about the reason this file must be edited again
 type editReason struct {
 	head  string
@@ -361,12 +366,15 @@ func (h *editHeader) writeTo(w io.Writer) error {
 	return nil
 }
 
+func (h *editHeader) flush() {
+	h.reasons = []editReason{}
+}
+
 // editResults capture the result of an update
 type editResults struct {
 	header    editHeader
 	retryable int
 	notfound  int
-	conflict  int
 	edit      []*resource.Info
 	file      string
 
@@ -378,23 +386,23 @@ func (r *editResults) addError(err error, info *resource.Info) string {
 	case errors.IsInvalid(err):
 		r.edit = append(r.edit, info)
 		reason := editReason{
-			head: fmt.Sprintf("%s %s was not valid", info.Mapping.Kind, info.Name),
+			head: fmt.Sprintf("%s %q was not valid", info.Mapping.Resource, info.Name),
 		}
 		if err, ok := err.(errors.APIStatus); ok {
 			if details := err.Status().Details; details != nil {
 				for _, cause := range details.Causes {
-					reason.other = append(reason.other, cause.Message)
+					reason.other = append(reason.other, fmt.Sprintf("%s: %s", cause.Field, cause.Message))
 				}
 			}
 		}
 		r.header.reasons = append(r.header.reasons, reason)
-		return fmt.Sprintf("Error: the %s %s is invalid", info.Mapping.Kind, info.Name)
+		return fmt.Sprintf("Error: %s %q is invalid", info.Mapping.Resource, info.Name)
 	case errors.IsNotFound(err):
 		r.notfound++
-		return fmt.Sprintf("Error: the %s %s could not be found on the server", info.Mapping.Kind, info.Name)
+		return fmt.Sprintf("Error: %s %q could not be found on the server", info.Mapping.Resource, info.Name)
 	default:
 		r.retryable++
-		return fmt.Sprintf("Error: the %s %s could not be patched: %v", info.Mapping.Kind, info.Name, err)
+		return fmt.Sprintf("Error: %s %q could not be patched: %v", info.Mapping.Resource, info.Name, err)
 	}
 }
 
@@ -432,7 +440,8 @@ func hasLines(r io.Reader) (bool, error) {
 // in it. Note that if the given file has a syntax error, the transformation will
 // fail and we will manually drop all comments from the file.
 func stripComments(file []byte) []byte {
-	stripped, err := yaml.ToJSON(file)
+	stripped := file
+	stripped, err := yaml.ToJSON(stripped)
 	if err != nil {
 		stripped = manualStrip(file)
 	}
@@ -442,12 +451,15 @@ func stripComments(file []byte) []byte {
 // manualStrip is used for dropping comments from a YAML file
 func manualStrip(file []byte) []byte {
 	stripped := []byte{}
-	for _, line := range bytes.Split(file, []byte("\n")) {
+	lines := bytes.Split(file, []byte("\n"))
+	for i, line := range lines {
 		if bytes.HasPrefix(bytes.TrimSpace(line), []byte("#")) {
 			continue
 		}
 		stripped = append(stripped, line...)
-		stripped = append(stripped, '\n')
+		if i < len(lines)-1 {
+			stripped = append(stripped, '\n')
+		}
 	}
 	return stripped
 }