diff --git a/pkg/kubectl/cmd/edit.go b/pkg/kubectl/cmd/edit.go index 69d92c4d23c..48ae40f5185 100644 --- a/pkg/kubectl/cmd/edit.go +++ b/pkg/kubectl/cmd/edit.go @@ -131,7 +131,7 @@ func runEdit(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args return err } - mapper, resourceMapper, r, cmdNamespace, err := getMapperAndResult(f, args, options, editMode) + mapper, originalResult, updatedResultsGetter, cmdNamespace, err := getMapperAndResult(f, args, options, editMode) if err != nil { return err } @@ -147,17 +147,12 @@ func runEdit(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args return err } - normalEditInfos, err := r.Infos() - if err != nil { - return err - } - var ( windowsLineEndings = cmdutil.GetFlagBool(cmd, "windows-line-endings") edit = editor.NewDefaultEditor(f.EditorEnvs()) ) - editFn := func(info *resource.Info, err error) error { + editFn := func(infos []*resource.Info) error { var ( results = editResults{} original = []byte{} @@ -166,16 +161,7 @@ func runEdit(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args ) containsError := false - var infos []*resource.Info for { - switch editMode { - case NormalEditMode: - infos = normalEditInfos - case EditBeforeCreateMode: - infos = []*resource.Info{info} - default: - err = fmt.Errorf("Not supported edit mode %q", editMode) - } originalObj, err := resource.AsVersionedObject(infos, false, defaultVersion, encoder) if err != nil { return err @@ -212,17 +198,10 @@ func runEdit(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args if err != nil { return preservedFile(err, results.file, errOut) } - if editMode == NormalEditMode || containsError { - 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 :) - return preservedFile(fmt.Errorf("%s", "Edit cancelled, no valid changes were saved."), file, errOut) - } + // If we're retrying the loop because of an error, and no change was made in the file, short-circuit + if containsError && bytes.Equal(stripComments(editedDiff), stripComments(edited)) { + return preservedFile(fmt.Errorf("%s", "Edit cancelled, no valid changes were saved."), file, errOut) } - // cleanup any file from the previous pass if len(results.file) > 0 { os.Remove(results.file) @@ -266,38 +245,37 @@ func runEdit(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args } // parse the edited file - updates, err := resourceMapper.InfoForData(edited, "edited-file") - if err != nil { + var updatedVisitor resource.Visitor + if updatedInfos, err := updatedResultsGetter(edited).Infos(); err != nil { // 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 + } else { + updatedVisitor = resource.InfoListVisitor(updatedInfos) } // not a syntax error as it turns out... containsError = false - namespaceVisitor := resource.NewFlattenListVisitor(updates, resourceMapper) + namespaceVisitor := updatedVisitor // need to make sure the original namespace wasn't changed while editing if err = namespaceVisitor.Visit(resource.RequireNamespace(cmdNamespace)); err != nil { return preservedFile(err, file, errOut) } // iterate through all items to apply annotations - mutatedObjects, err := visitAnnotation(cmd, f, updates, resourceMapper, encoder) - if err != nil { + annotationVisitor := updatedVisitor + if _, err := visitAnnotation(cmd, f, annotationVisitor, encoder); err != nil { return preservedFile(err, file, errOut) } - // if we mutated a list in the visitor, persist the changes on the overall object - if meta.IsListType(updates.Object) { - meta.SetList(updates.Object, mutatedObjects) - } - switch editMode { case NormalEditMode: - err = visitToPatch(originalObj, updates, mapper, resourceMapper, encoder, out, errOut, defaultVersion, &results, file) + patchVisitor := updatedVisitor + err = visitToPatch(originalObj, patchVisitor, mapper, encoder, out, errOut, defaultVersion, &results, file) case EditBeforeCreateMode: - err = visitToCreate(updates, mapper, resourceMapper, out, errOut, defaultVersion, &results, file) + createVisitor := updatedVisitor + err = visitToCreate(createVisitor, mapper, out, errOut, defaultVersion, &results, file) default: err = fmt.Errorf("Not supported edit mode %q", editMode) } @@ -337,10 +315,16 @@ func runEdit(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args switch editMode { // If doing normal edit we cannot use Visit because we need to edit a list for convenience. Ref: #20519 case NormalEditMode: - return editFn(nil, nil) + infos, err := originalResult.Infos() + if err != nil { + return err + } + return editFn(infos) // If doing an edit before created, we don't want a list and instead want the normal behavior as kubectl create. case EditBeforeCreateMode: - return r.Visit(editFn) + return originalResult.Visit(func(info *resource.Info, err error) error { + return editFn([]*resource.Info{info}) + }) default: return fmt.Errorf("Not supported edit mode %q", editMode) } @@ -366,7 +350,9 @@ func getPrinter(cmd *cobra.Command) (*editPrinterOptions, error) { } } -func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.FilenameOptions, editMode EditMode) (meta.RESTMapper, *resource.Mapper, *resource.Result, string, error) { +type resultGetter func([]byte) *resource.Result + +func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.FilenameOptions, editMode EditMode) (meta.RESTMapper, *resource.Result, resultGetter, string, error) { cmdNamespace, enforceNamespace, err := f.DefaultNamespace() if err != nil { return nil, nil, nil, "", err @@ -384,18 +370,8 @@ func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.File if err != nil { return nil, nil, nil, "", err } - resourceMapper := &resource.Mapper{ - ObjectTyper: typer, - RESTMapper: mapper, - ClientMapper: resource.ClientMapperFunc(f.ClientForMapping), - // NB: we use `f.Decoder(false)` to get a plain deserializer for - // the resourceMapper, since it's used to read in edits and - // we don't want to convert into the internal version when - // reading in edits (this would cause us to potentially try to - // compare two different GroupVersions). - Decoder: f.Decoder(false), - } + // resource builder to read objects from the original file or arg invocation var b *resource.Builder switch editMode { case NormalEditMode: @@ -407,31 +383,42 @@ func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.File default: return nil, nil, nil, "", fmt.Errorf("Not supported edit mode %q", editMode) } - r := b.NamespaceParam(cmdNamespace).DefaultNamespace(). + + originalResult := b.NamespaceParam(cmdNamespace).DefaultNamespace(). FilenameParam(enforceNamespace, options). ContinueOnError(). Flatten(). Do() - err = r.Err() + err = originalResult.Err() if err != nil { return nil, nil, nil, "", err } - return mapper, resourceMapper, r, cmdNamespace, err + + updatedResultGetter := func(data []byte) *resource.Result { + // resource builder to read objects from the original file or arg invocation + var b *resource.Builder + switch editMode { + case NormalEditMode: + b = resource.NewBuilder(mapper, typer, resource.ClientMapperFunc(f.ClientForMapping), f.Decoder(true)) + case EditBeforeCreateMode: + b = resource.NewBuilder(mapper, typer, resource.ClientMapperFunc(f.UnstructuredClientForMapping), unstructured.UnstructuredJSONScheme) + } + return b.Stream(bytes.NewReader(data), "edited-file").ContinueOnError().Flatten().Do() + } + + return mapper, originalResult, updatedResultGetter, cmdNamespace, err } func visitToPatch( originalObj runtime.Object, - updates *resource.Info, + patchVisitor resource.Visitor, mapper meta.RESTMapper, - resourceMapper *resource.Mapper, encoder runtime.Encoder, out, errOut io.Writer, defaultVersion schema.GroupVersion, results *editResults, file string, ) error { - - patchVisitor := resource.NewFlattenListVisitor(updates, resourceMapper) err := patchVisitor.Visit(func(info *resource.Info, incomingErr error) error { currOriginalObj := originalObj @@ -515,8 +502,7 @@ func visitToPatch( return err } -func visitToCreate(updates *resource.Info, mapper meta.RESTMapper, resourceMapper *resource.Mapper, out, errOut io.Writer, defaultVersion schema.GroupVersion, results *editResults, file string) error { - createVisitor := resource.NewFlattenListVisitor(updates, resourceMapper) +func visitToCreate(createVisitor resource.Visitor, mapper meta.RESTMapper, out, errOut io.Writer, defaultVersion schema.GroupVersion, results *editResults, file string) error { err := createVisitor.Visit(func(info *resource.Info, incomingErr error) error { results.version = defaultVersion if err := createAndRefresh(info); err != nil { @@ -528,9 +514,8 @@ func visitToCreate(updates *resource.Info, mapper meta.RESTMapper, resourceMappe return err } -func visitAnnotation(cmd *cobra.Command, f cmdutil.Factory, updates *resource.Info, resourceMapper *resource.Mapper, encoder runtime.Encoder) ([]runtime.Object, error) { +func visitAnnotation(cmd *cobra.Command, f cmdutil.Factory, annotationVisitor resource.Visitor, encoder runtime.Encoder) ([]runtime.Object, error) { mutatedObjects := []runtime.Object{} - annotationVisitor := resource.NewFlattenListVisitor(updates, resourceMapper) // iterate through all items to apply annotations err := annotationVisitor.Visit(func(info *resource.Info, incomingErr error) error { // put configuration annotation in "updates" diff --git a/pkg/kubectl/cmd/edit_test.go b/pkg/kubectl/cmd/edit_test.go index eb073fa2c87..6826bf19f2b 100644 --- a/pkg/kubectl/cmd/edit_test.go +++ b/pkg/kubectl/cmd/edit_test.go @@ -221,6 +221,20 @@ func TestEdit(t *testing.T) { Client: fake.CreateHTTPClient(reqResp), }, nil } + tf.UnstructuredClientForMappingFunc = func(mapping *meta.RESTMapping) (resource.RESTClient, error) { + versionedAPIPath := "" + if mapping.GroupVersionKind.Group == "" { + versionedAPIPath = "/api/" + mapping.GroupVersionKind.Version + } else { + versionedAPIPath = "/apis/" + mapping.GroupVersionKind.Group + "/" + mapping.GroupVersionKind.Version + } + return &fake.RESTClient{ + APIRegistry: api.Registry, + VersionedAPIPath: versionedAPIPath, + NegotiatedSerializer: unstructuredSerializer, + Client: fake.CreateHTTPClient(reqResp), + }, nil + } if len(testcase.Namespace) > 0 { tf.Namespace = testcase.Namespace diff --git a/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/1.request b/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/1.request index ce3eabeccc8..8b60bbb47b6 100755 --- a/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/1.request +++ b/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/1.request @@ -1,31 +1,32 @@ { - "kind": "Service", "apiVersion": "v1", + "kind": "Service", "metadata": { - "name": "svc1", - "namespace": "edit-test", - "selfLink": "/api/v1/namespaces/edit-test/services/svc1", - "uid": "4149f70e-e9dc-11e6-8c3b-acbc32c1ca87", "creationTimestamp": "2017-02-03T06:44:47Z", "labels": { "app": "svc1modified" - } + }, + "name": "svc1", + "namespace": "edit-test", + "resourceVersion": "", + "selfLink": "/api/v1/namespaces/edit-test/services/svc1", + "uid": "4149f70e-e9dc-11e6-8c3b-acbc32c1ca87" }, "spec": { + "clusterIP": "10.0.0.118", "ports": [ { "name": "81", - "protocol": "TCP", "port": 82, + "protocol": "TCP", "targetPort": 81 } ], "selector": { "app": "svc1" }, - "clusterIP": "10.0.0.118", - "type": "ClusterIP", - "sessionAffinity": "None" + "sessionAffinity": "None", + "type": "ClusterIP" }, "status": { "loadBalancer": {} diff --git a/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/3.request b/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/3.request index 38cee1a5684..fb9c3d1c0ed 100755 --- a/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/3.request +++ b/pkg/kubectl/cmd/testdata/edit/testcase-create-list-error/3.request @@ -1,31 +1,32 @@ { - "kind": "Service", "apiVersion": "v1", + "kind": "Service", "metadata": { - "name": "svc2", - "namespace": "edit-test", - "selfLink": "/api/v1/namespaces/edit-test/services/svc2", - "uid": "3e9b10db-e9dc-11e6-8c3b-acbc32c1ca87", "creationTimestamp": "2017-02-03T06:44:43Z", "labels": { "app": "svc2modified" - } + }, + "name": "svc2", + "namespace": "edit-test", + "resourceVersion": "", + "selfLink": "/api/v1/namespaces/edit-test/services/svc2", + "uid": "3e9b10db-e9dc-11e6-8c3b-acbc32c1ca87" }, "spec": { + "clusterIP": "10.0.0.182.1", "ports": [ { "name": "80", - "protocol": "VHF", "port": 80, + "protocol": "VHF", "targetPort": 80 } ], "selector": { "app": "svc2" }, - "clusterIP": "10.0.0.182.1", - "type": "ClusterIP", - "sessionAffinity": "None" + "sessionAffinity": "None", + "type": "ClusterIP" }, "status": { "loadBalancer": {} diff --git a/pkg/kubectl/cmd/testdata/edit/testcase-create-list/1.request b/pkg/kubectl/cmd/testdata/edit/testcase-create-list/1.request index dde2695b53b..39e94fef426 100755 --- a/pkg/kubectl/cmd/testdata/edit/testcase-create-list/1.request +++ b/pkg/kubectl/cmd/testdata/edit/testcase-create-list/1.request @@ -1,31 +1,27 @@ { - "kind": "Service", "apiVersion": "v1", + "kind": "Service", "metadata": { - "name": "svc1", - "namespace": "edit-test", - "creationTimestamp": null, "labels": { "app": "svc1", "new-label": "new-value" - } + }, + "name": "svc1", + "namespace": "edit-test" }, "spec": { "ports": [ { "name": "81", - "protocol": "TCP", "port": 82, + "protocol": "TCP", "targetPort": 81 } ], "selector": { "app": "svc1" }, - "type": "ClusterIP", - "sessionAffinity": "None" - }, - "status": { - "loadBalancer": {} + "sessionAffinity": "None", + "type": "ClusterIP" } } diff --git a/pkg/kubectl/cmd/testdata/edit/testcase-create-list/3.request b/pkg/kubectl/cmd/testdata/edit/testcase-create-list/3.request index 9c71a88f4f0..91fd5ae2409 100755 --- a/pkg/kubectl/cmd/testdata/edit/testcase-create-list/3.request +++ b/pkg/kubectl/cmd/testdata/edit/testcase-create-list/3.request @@ -1,20 +1,19 @@ { - "kind": "Service", "apiVersion": "v1", + "kind": "Service", "metadata": { - "name": "svc2", - "namespace": "edit-test", - "creationTimestamp": null, "labels": { "app": "svc2" - } + }, + "name": "svc2", + "namespace": "edit-test" }, "spec": { "ports": [ { "name": "80", - "protocol": "TCP", "port": 80, + "protocol": "TCP", "targetPort": 81 } ], @@ -22,10 +21,7 @@ "app": "svc2", "new-label": "new-value" }, - "type": "ClusterIP", - "sessionAffinity": "None" - }, - "status": { - "loadBalancer": {} + "sessionAffinity": "None", + "type": "ClusterIP" } } diff --git a/pkg/kubectl/cmd/testdata/edit/testcase-syntax-error/2.edited b/pkg/kubectl/cmd/testdata/edit/testcase-syntax-error/2.edited index c56febe9f8f..7f63a24ce56 100755 --- a/pkg/kubectl/cmd/testdata/edit/testcase-syntax-error/2.edited +++ b/pkg/kubectl/cmd/testdata/edit/testcase-syntax-error/2.edited @@ -2,7 +2,7 @@ # and an empty file will abort the edit. If an error occurs while saving this file will be # reopened with the relevant failures. # -# The edited file had a syntax error: unable to decode "edited-file": yaml: line 17: could not find expected ':' +# The edited file had a syntax error: error converting YAML to JSON: yaml: line 17: could not find expected ':' # apiVersion: v1 kind: Service diff --git a/pkg/kubectl/cmd/testdata/edit/testcase-syntax-error/2.original b/pkg/kubectl/cmd/testdata/edit/testcase-syntax-error/2.original index ea21819b1fa..85da34d2382 100755 --- a/pkg/kubectl/cmd/testdata/edit/testcase-syntax-error/2.original +++ b/pkg/kubectl/cmd/testdata/edit/testcase-syntax-error/2.original @@ -2,7 +2,7 @@ # and an empty file will abort the edit. If an error occurs while saving this file will be # reopened with the relevant failures. # -# The edited file had a syntax error: unable to decode "edited-file": yaml: line 17: could not find expected ':' +# The edited file had a syntax error: error converting YAML to JSON: yaml: line 17: could not find expected ':' # apiVersion: v1 kind: Service diff --git a/pkg/kubectl/resource/visitor.go b/pkg/kubectl/resource/visitor.go index 32102d4ed17..7cae799c606 100644 --- a/pkg/kubectl/resource/visitor.go +++ b/pkg/kubectl/resource/visitor.go @@ -695,3 +695,13 @@ func FilterBySelector(s labels.Selector) FilterFunc { return true, nil } } + +type InfoListVisitor []*Info + +func (infos InfoListVisitor) Visit(fn VisitorFunc) error { + var err error + for _, i := range infos { + err = fn(i, err) + } + return err +}