Give "edit instancegroup" parity with "edit cluster"

cmd/kops/edit_instancegroup.go

@@ -23,6 +23,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/spf13/cobra"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -31,6 +32,7 @@ import (
 	api "k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/pkg/apis/kops/validation"
 	"k8s.io/kops/pkg/assets"
+	"k8s.io/kops/pkg/edit"
 	"k8s.io/kops/pkg/kopscodecs"
 	"k8s.io/kops/pkg/try"
 	"k8s.io/kops/upup/pkg/fi/cloudup"
@@ -126,7 +128,7 @@ func RunEditInstanceGroup(ctx context.Context, f *util.Factory, out io.Writer, o
 	}
 
 	var (
-		edit = editor.NewDefaultEditor(editorEnvs)
+		editor = editor.NewDefaultEditor(editorEnvs)
 	)
 
 	ext := "yaml"
@@ -135,64 +137,142 @@ func RunEditInstanceGroup(ctx context.Context, f *util.Factory, out io.Writer, o
 		return err
 	}
 
-	// launch the editor
-	edited, file, err := edit.LaunchTempFile(fmt.Sprintf("%s-edit-", filepath.Base(os.Args[0])), ext, bytes.NewReader(raw))
-	defer func() {
-		if file != "" {
-			try.RemoveFile(file)
-		}
-	}()
-	if err != nil {
-		return fmt.Errorf("error launching editor: %v", err)
-	}
+	var (
+		results = editResults{}
+		edited  = []byte{}
+		file    string
+	)
+
+	containsError := false
+
+	for {
+		buf := &bytes.Buffer{}
+		results.header.writeTo(buf)
+		results.header.flush()
+
+		if !containsError {
+			buf.Write(raw)
+		} else {
+			buf.Write(stripComments(edited))
+		}
+
+		// launch the editor
+		editedDiff := edited
+		edited, file, err = editor.LaunchTempFile(fmt.Sprintf("%s-edit-", filepath.Base(os.Args[0])), ext, buf)
+		if err != nil {
+			return preservedFile(fmt.Errorf("error launching editor: %v", err), results.file, out)
+		}
+
+		if containsError {
+			if bytes.Equal(stripComments(editedDiff), stripComments(edited)) {
+				return preservedFile(fmt.Errorf("%s", "Edit cancelled: no valid changes were saved."), file, out)
+			}
+		}
+
+		if len(results.file) > 0 {
+			try.RemoveFile(results.file)
+		}
+
+		if bytes.Equal(stripComments(raw), stripComments(edited)) {
+			try.RemoveFile(file)
+			fmt.Fprintln(out, "Edit cancelled: no changes made.")
+			return nil
+		}
+
+		lines, err := hasLines(bytes.NewBuffer(edited))
+		if err != nil {
+			return preservedFile(err, file, out)
+		}
+		if !lines {
+			try.RemoveFile(file)
+			fmt.Fprintln(out, "Edit cancelled: saved file was empty.")
+			return nil
+		}
+
+		newObj, _, err := kopscodecs.Decode(edited, nil)
+		if err != nil {
+			return preservedFile(fmt.Errorf("error parsing InstanceGroup: %v", err), file, out)
+		}
+
+		newGroup, ok := newObj.(*api.InstanceGroup)
+		if !ok {
+			results = editResults{
+				file: file,
+			}
+			results.header.addError(fmt.Sprintf("object was not of expected type: %T", newObj))
+			containsError = true
+			continue
+		}
+
+		extraFields, err := edit.HasExtraFields(string(edited), newObj)
+		if err != nil {
+			results = editResults{
+				file: file,
+			}
+			results.header.addError(fmt.Sprintf("error checking for extra fields: %v", err))
+			containsError = true
+			continue
+		}
+		if extraFields != "" {
+			results = editResults{
+				file: file,
+			}
+			lines := strings.Split(extraFields, "\n")
+			for _, line := range lines {
+				results.header.addExtraFields(line)
+			}
+			containsError = true
+			continue
+		}
+
+		cloud, err := cloudup.BuildCloud(cluster)
+		if err != nil {
+			return err
+		}
+		fullGroup, err := cloudup.PopulateInstanceGroupSpec(cluster, newGroup, cloud, channel)
+		if err != nil {
+			results = editResults{
+				file: file,
+			}
+			results.header.addError(fmt.Sprintf("error populating instance group spec: %s", err))
+			containsError = true
+			continue
+		}
+
+		// We need the full cluster spec to perform deep validation
+		// Note that we don't write it back though
+		err = cloudup.PerformAssignments(cluster, cloud)
+		if err != nil {
+			return preservedFile(fmt.Errorf("error populating configuration: %v", err), file, out)
+		}
+
+		assetBuilder := assets.NewAssetBuilder(cluster, false)
+		fullCluster, err := cloudup.PopulateClusterSpec(clientset, cluster, cloud, assetBuilder)
+		if err != nil {
+			results = editResults{
+				file: file,
+			}
+			results.header.addError(fmt.Sprintf("error populating cluster spec: %s", err))
+			containsError = true
+			continue
+		}
+
+		err = validation.CrossValidateInstanceGroup(fullGroup, fullCluster, cloud).ToAggregate()
+		if err != nil {
+			results = editResults{
+				file: file,
+			}
+			results.header.addError(fmt.Sprintf("validation failed: %s", err))
+			containsError = true
+			continue
+		}
+
+		// Note we perform as much validation as we can, before writing a bad config
+		_, err = clientset.InstanceGroupsFor(cluster).Update(ctx, fullGroup, metav1.UpdateOptions{})
+		if err != nil {
+			return preservedFile(err, file, out)
+		}
 
-	if bytes.Equal(edited, raw) {
-		fmt.Fprintln(out, "Edit cancelled: no changes made.")
 		return nil
 	}
-
-	newObj, _, err := kopscodecs.Decode(edited, nil)
-	if err != nil {
-		return fmt.Errorf("error parsing InstanceGroup: %v", err)
-	}
-
-	newGroup, ok := newObj.(*api.InstanceGroup)
-	if !ok {
-		return fmt.Errorf("object was not of expected type: %T", newObj)
-	}
-
-	cloud, err := cloudup.BuildCloud(cluster)
-	if err != nil {
-		return err
-	}
-	fullGroup, err := cloudup.PopulateInstanceGroupSpec(cluster, newGroup, cloud, channel)
-	if err != nil {
-		return err
-	}
-
-	// We need the full cluster spec to perform deep validation
-	// Note that we don't write it back though
-	err = cloudup.PerformAssignments(cluster, cloud)
-	if err != nil {
-		return fmt.Errorf("error populating configuration: %v", err)
-	}
-
-	assetBuilder := assets.NewAssetBuilder(cluster, false)
-	fullCluster, err := cloudup.PopulateClusterSpec(clientset, cluster, cloud, assetBuilder)
-	if err != nil {
-		return err
-	}
-
-	err = validation.CrossValidateInstanceGroup(fullGroup, fullCluster, cloud).ToAggregate()
-	if err != nil {
-		return err
-	}
-
-	// Note we perform as much validation as we can, before writing a bad config
-	_, err = clientset.InstanceGroupsFor(cluster).Update(ctx, fullGroup, metav1.UpdateOptions{})
-	if err != nil {
-		return err
-	}
-
-	return nil
 }